Skip to content
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

docs: Clarify assert.doesNotThrow behavior #2807

Closed

Conversation

foliveira
Copy link
Contributor

Relates to #2385

The documentation now seems to be up to date with the code and tests for the assert module

@ChALkeR ChALkeR added doc Issues and PRs related to the documentations. assert Issues and PRs related to the assert subsystem. labels Sep 11, 2015

Expects `block` not to throw an error, see `assert.throws` for details.
Expects `block` not to throw an error, see `assert.throws` for more details.
Copy link
Member

Choose a reason for hiding this comment

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

While we're at it, can we fix the comma splice here? This should be two sentences. ...not to throw an error. See...

@foliveira
Copy link
Contributor Author

@Trott updated the branch with your suggestions


Expects `block` not to throw an error, see `assert.throws` for details.
Expects `block` not to throw an error. See `assert.throws` for more details.
Copy link
Member

Choose a reason for hiding this comment

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

Two more things (sorry!):

Should probably be assert.throws() with the parentheses, and that text should probably link to the section of the document for assert.throws().

function() {
throw new TypeError("Wrong value");
},
a.AssertionError
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be clearer if you stayed away from AssertionError. I'd go with maybe SyntaxError.

@Trott
Copy link
Member

Trott commented Sep 11, 2015

I made several suggested changes with inline comments, but LGTM. /cc @nodejs/documentation

@foliveira
Copy link
Contributor Author

@Trott thanks for the tips/suggestions. All should be fixed now.

@Trott
Copy link
Member

Trott commented Sep 11, 2015

👍

LGTM

The documentation for assert.doesNotThrow now reflects all the inputs the
function accepts, as well as the errors thrown for each combination of
parameter types.
@foliveira
Copy link
Contributor Author

Pushed rebased commits

@Trott
Copy link
Member

Trott commented Sep 14, 2015

LGTM. It would be good if someone from @nodejs/documentation could look at it just to make sure it conforms with any emerging or established documentation guidelines that I may be unaware of.

Trott pushed a commit that referenced this pull request Sep 23, 2015
The documentation for assert.doesNotThrow now reflects all the inputs
the function accepts, as well as the errors thrown for each combination
of parameter types.

PR-URL: #2807
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Sep 23, 2015

Landed in 79ebeab. Thanks!

@Trott Trott closed this Sep 23, 2015
Fishrock123 pushed a commit that referenced this pull request Sep 25, 2015
The documentation for assert.doesNotThrow now reflects all the inputs
the function accepts, as well as the errors thrown for each combination
of parameter types.

PR-URL: #2807
Reviewed-By: Rich Trott <[email protected]>
This was referenced Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants