-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: ResultBytes #86
Conversation
- Add ResultBytes to sqlitex to support one-result BLOB queries - Export ErrNoResults/ErrMultipleResults so the user can react to these conditions programmatically.
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.
I'm surprised this function didn't already exist, so I would be delighted to merge this in. 😄 Just a few tweaks, and we should be good to go. Thanks for the PR!
sqlitex/query.go
Outdated
var ( | ||
ErrNoResults = errors.New("sqlite: statement has no results") | ||
ErrMultipleResults = errors.New("sqlite: statement has multiple result rows") | ||
) |
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.
Let's revert this change.
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.
Can I get some clarification on this? Generally, I believe that sentinel errors should be exported if they have meaningful names and convey meaningful information. In this case, they would allow the caller the option to distinguish between "the call I explicitly expected to have one result has none" and " the call I explicitly expected to have one result has more than one".
If the argument is that you deliberately want this information to be opaque, I can get behind that I suppose. The semantics of the methods that might return these errors are clear enough ("one result, or error"). That would make these errors not so much sentinel errors but helpful templates. I just want to make sure I'm clear on the reasoning behind the decision to hide this distinction.
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.
Yup, sorry for being terse. I'm asking to revert from the standpoint that ResultBytes
is useful on its own (and the title/focus of the PR) and I would like to merge it in. I am less sure about exporting the errors at this time, so would like to figure that outside the scope of this PR.
Oh, and mind adding a small test for this function? |
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.
I'm carrying this forward with the changes I requested so we can get this in.
conditions programmatically.
(ref: Document pattern of checking for zero rows #85)