-
Notifications
You must be signed in to change notification settings - Fork 909
GODRIVER-1802 add ServerError interface #549
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
Conversation
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.
Looking good! Just have a few questions. I'm not completely sure about the usefulness of Unwrap
either.
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.
LGTM!
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.
Code changes look good! I'm really happy with how this new API is coming together. The main thing missing here is documentation. There is a planned effort for the docs team to create reference docs for the driver, but we don't know when that'll be, so we should at least add a section to mongo/doc.go
. I'm fine making this a follow-on ticket, but it should at least be a requirement for this project.
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.
added a little reference to ServerErrror to mongo/doc.go. I can make it a more full fledged section if that seems useful, but I thought that was enough to get users to look if they need it.
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 think the docs for the ServerError
type in mongo/errors.go
should mention that it can represent one or more errors (e.g. for use-cases like BulkWrite
or InsertMany
, it can represent a number of write errors and/or a write concern failure) and that some of the interface functions like HasMessage
will return true if any of the individual errors satisfies the check.
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.
LGTM!! 🎊
Still not sure about add Unwrap to the interface when CommandError is the only one that actually uses it.