Skip to content

docs(NODE-3363): added docs for proposed error hierarchy #2862

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 24 commits into from
Jul 7, 2021

Conversation

andymina
Copy link
Contributor

Description

  • Added docs/errors.md which details the proposed new error class hierarchy.
  • Added examples of when each error class may be used.

@andymina andymina changed the title Node 3363/4.0/error hierarchy docs(NODE-3363): added markdown detailing proposed hierarchy Jun 25, 2021
@andymina andymina changed the title docs(NODE-3363): added markdown detailing proposed hierarchy docs(NODE-3363): added docs for proposed error hierarchy Jun 25, 2021
@W-A-James W-A-James added the wip label Jun 25, 2021
@W-A-James W-A-James marked this pull request as ready for review June 25, 2021 20:53
@andymina andymina requested review from nbbeeken and emadum June 25, 2021 20:55
@nbbeeken nbbeeken added Team Review Needs review from team and removed wip labels Jun 25, 2021
@nbbeeken nbbeeken requested a review from durran June 25, 2021 20:58
@@ -95,6 +95,38 @@ describe('GridFS Stream', function () {
}
});

it('destroy publishes provided error', {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this test here doing?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nbbeeken I think this test was here already, but it got scooped into this PR after we rebased onto the most recent 4.0 branch. We may have some git-fu to figure out to get rid of that change in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave this comment open until the mysterious test is taken care of?

@andymina andymina requested a review from nbbeeken June 29, 2021 18:31
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

Had a few points of discussion but this is looking great, nice job! 👍

I noticed some of the errors in the graphs, like MongoNoCursorInChangeStreamError, aren't also in the markdown text. Would you be able to add any errors that are only in the graph to the markdown, so that we can reference them more easily in the review?

docs/errors.md Outdated

## `MongoNetworkError`

These are errors encountered at runtime which occur when the driver encounters an issue in the network which leads to an inability to connect to a mongo server instance. Children of this class include:
Copy link
Contributor

Choose a reason for hiding this comment

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

We should find a way to phrase it as less of a run on sentence

docs/errors.md Outdated

- #### `MongoNetworkTimeoutError`

- Thrown when a timeout expires in attempting to connect to the mongo server
Copy link
Contributor

@dariakp dariakp Jul 6, 2021

Choose a reason for hiding this comment

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

Suggested change
- Thrown when a timeout expires in attempting to connect to the mongo server
- Thrown when a timeout expires while attempting to connect to the mongo server.

Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for all the work here!

@nbbeeken nbbeeken removed the request for review from durran July 7, 2021 20:20
@andymina andymina force-pushed the NODE-3363/4.0/error-hierarchy branch from b3e6a38 to 0d69383 Compare July 7, 2021 20:44
@nbbeeken nbbeeken merged commit 063ea23 into 4.0 Jul 7, 2021
@nbbeeken nbbeeken deleted the NODE-3363/4.0/error-hierarchy branch July 7, 2021 21:27
ljhaywar pushed a commit that referenced this pull request Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants