Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lee #13

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Lee #13

wants to merge 9 commits into from

Conversation

chilledout71
Copy link

No description provided.

@chilledout71 chilledout71 requested a review from wildeng March 21, 2022 14:59
Copy link

@wildeng wildeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides a couple of mainly nit comments, I found interesting the use of a DataStore. I think there will be a good debate where the some logic should belong. I put it in the model, for example, given my past experiences - with Java too - so I think we should agree how to deal with this kind of situation


books = repo.GetBooks(ctx, filters)

sort.Slice(books, func(i, j int) bool { return books[i].Id < books[j].Id })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found that later, meant to change and forgot

return
}

jsonStr, err := json.MarshalIndent(book, "", " ")
if err != nil {
utils.ErrorResponse(w, err)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this 🥳 utils package


server.Init(host, port)
server.Init(host, port, client, ctx)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return createBooks(it)
}

func (brds *BookRepositoryDataStore) Lending(ctx context.Context, id int, borrow bool) (err error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I understood what the endpoint does, but I found the name a bit confusing what about loan?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh I struggled with the name

func buildQuery(filters map[string]string) *datastore.Query {
query := datastore.NewQuery("Book")
for filter, value := range filters {
if strings.EqualFold("title", filter) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would you modify the method to filter by author and title ? @tomw-1 this is your question 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does both already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants