Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errorf()
's return values have the methodGRPCStatus() *Status
; however, this is not documented anywhere. It should be explicit that this function turnserror
s produced byErrorf
or(*Status).Err()
into*Status
es, as that is its primary function.Why not something more like the original text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end it's up to you. My goal by changing that was to make it very explicit that we return
ok=true
forerr==nil
because that can be against intuition if you are under the impression this works like a type assertion.Take the following code fragment as example:
Feel free to change this PR if you have an idea about how to improve it better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text I proposed above seems pretty explicit that
ok=true
whenerr=nil
, as that case is documented above the "Otherwise, ok is false" sentence.Your example code is highly unusual. Why would you test
err
to see what type it is before checking to see if there even was an error by comparing withnil
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems your judgement about this issue is different from mine. I really don't mind and accept that and will close this PR. There are probably more important fixes waiting than this.
Feel free to close the linked issue and tag it with "Wontfix".