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

my attempt #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

my attempt #5

wants to merge 1 commit into from

Conversation

rmpugh
Copy link

@rmpugh rmpugh commented Feb 7, 2022

No description provided.

@rmpugh rmpugh requested a review from tomw-1 February 17, 2022 10:03
Copy link
Collaborator

@tomw-1 tomw-1 left a comment

Choose a reason for hiding this comment

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

noice

return nil, err
} else if book == nil {
w.WriteHeader(http.StatusNotFound)
return nil, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

if book is nil, return an err, not nil, nil. From what i've seen in go, returning nil objects isn't a convention. Better to return a not found err and let the next level up decide what to do with it

Copy link
Collaborator

Choose a reason for hiding this comment

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

its a slightly old post, but i think the convention holds (https://go.dev/blog/go1.13-errors)

@@ -9,4 +9,4 @@ services:
ports:
- "3031:3031"
volumes:
- "~/.config/gcloud/emulators/datastore/:/opt/data"
- "~/.config/gcloud/emulators/store/:/opt/data"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a change that we need to make to master? or was it a local config thing?

}
it := client.Run(ctx, query)
for {
var itemProperties datastore.PropertyList
Copy link
Collaborator

Choose a reason for hiding this comment

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

:nit May only be a github formatting thing, but we should get in the habit of doing go fmt as a part of the save/commit cycle. Should avoid slightly odd indentation, and does things like realigning the structs to a standard convention

return update("Book", book.Id, book)
}

func createBookFromProperties(properties []datastore.Property) (Book, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this your version of generics!!

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