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

Greg's Books #4

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

Greg's Books #4

wants to merge 7 commits into from

Conversation

gr-eg
Copy link
Collaborator

@gr-eg gr-eg commented Jan 26, 2022

No description provided.


book, err := models.FindBookById(id)

if book.Borrowed == false {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the scenario that the err is not nil, then book is an 'empty' book object. Which would have a borrowed attribute set to its uninit'ed value, which is false. So by checking the borrowed flag before the error would mean any error in the look up would give a cannot return a book that is not borrowed error, rather than no such book error

"cloud.google.com/go/datastore"
)

var client *datastore.Client
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm surprised (and disappointed in Go) that this works. I would have thought it would have to be Client, not client to have visibility outside of its own file.

Copy link

@JamesBalazs JamesBalazs Feb 25, 2022

Choose a reason for hiding this comment

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

Capitalization denotes that the variable is exported from the package rather than the file - definitely makes it confusing when you see things using mystery variables defined in another file in the same package...

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.

3 participants