Skip to content

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

Merged
merged 7 commits into from
Dec 15, 2020
Merged

Conversation

iwysiu
Copy link
Contributor

@iwysiu iwysiu commented Dec 4, 2020

Still not sure about add Unwrap to the interface when CommandError is the only one that actually uses it.

Copy link
Contributor

@kevinAlbs kevinAlbs left a 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.

@iwysiu iwysiu requested a review from kevinAlbs December 7, 2020 22:59
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@divjotarora divjotarora left a 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.

Copy link
Contributor Author

@iwysiu iwysiu left a 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.

@iwysiu iwysiu requested a review from divjotarora December 14, 2020 16:23
Copy link
Contributor

@divjotarora divjotarora left a 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.

@iwysiu iwysiu requested a review from divjotarora December 14, 2020 16:57
@iwysiu iwysiu removed the request for review from craiggwilson December 15, 2020 16:29
Copy link
Contributor

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

LGTM!! 🎊

@iwysiu iwysiu merged commit 24febe4 into mongodb:master Dec 15, 2020
@iwysiu iwysiu deleted the GODRIVER-1802 branch December 15, 2020 19:36
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.

5 participants