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

doc: improve assert documentation #22692

Closed
wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 4, 2018

This fixes the officially accepted message types for assert.throws(),
assert.rejects(), assert.doesNotThrow() and
assert.doesNotReject(). It also renames the block argument in
those functions to fn and promiseFn for further clarity.

Errors would be ambiguous and were never intended to be special
handled as message. They are "accepted" in case it's the third argument
by coercing those to a string.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This fixes the officially accepted message types for `assert.throws()`,
`assert.rejects()`, `assert.doesNotThrow()` and
`assert.doesNotReject()`. It also renames the `block` argument in
those functions to `fn` and `promiseFn` for further clarity.
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Sep 4, 2018
If specified, `message` will be the message provided by the `AssertionError` if
the block fails to throw.
If specified, `message` will be appended to the message provided by the
`AssertionError` if the fn call fails to throw or in case the error validation
Copy link
Contributor

Choose a reason for hiding this comment

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

fn -> `fn`

@@ -408,19 +408,19 @@ parameter is undefined, a default error message is assigned. If the `message`
parameter is an instance of an [`Error`][] then it will be thrown instead of the
`AssertionError`.

## assert.doesNotReject(block[, error][, message])
## assert.doesNotReject(promiseFn[, error][, message])
Copy link
Member

Choose a reason for hiding this comment

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

How about asyncFn? That’s more or less the name the language settled on…

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 5, 2018

@gdams
Copy link
Member

gdams commented Sep 7, 2018

landed in: c1483ba

@gdams gdams closed this Sep 7, 2018
gdams pushed a commit that referenced this pull request Sep 7, 2018
This fixes the officially accepted message types for `assert.throws()`,
`assert.rejects()`, `assert.doesNotThrow()` and
`assert.doesNotReject()`. It also renames the `block` argument in
those functions to `fn` and `promiseFn` for further clarity.

PR-URL: #22692
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: George Adams <[email protected]>
@Trott
Copy link
Member

Trott commented Sep 8, 2018

The argument names should have been changed in the code as well or else not changed in the docs.

> assert.throws('hey', 'jude')
TypeError [ERR_INVALID_ARG_TYPE]: The "block" argument must be of type Function. Received type string
    at getActual (assert.js:560:11)
    at Function.throws (assert.js:680:24)
> 

The user doesn't have a way of knowing what the "block" argument is without looking at Node.js source code. They should be able to figure it out by looking at the docs. Before this change, they could. Now they can't.

@BridgeAR BridgeAR mentioned this pull request Sep 8, 2018
4 tasks
targos pushed a commit that referenced this pull request Sep 8, 2018
This fixes the officially accepted message types for `assert.throws()`,
`assert.rejects()`, `assert.doesNotThrow()` and
`assert.doesNotReject()`. It also renames the `block` argument in
those functions to `fn` and `promiseFn` for further clarity.

PR-URL: #22692
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: George Adams <[email protected]>
@BridgeAR BridgeAR deleted the improve-assert-docs branch January 20, 2020 11:37
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants