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: add warning to assert.doesNotThrow() #18699

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

Using assert.doesNotThrow() has no benefit over adding a comment
next to some code that should not throw. Therefore it is from now
on discouraged to use it.

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
Affected core subsystem(s)

doc, assert

@BridgeAR BridgeAR added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 10, 2018
@BridgeAR BridgeAR requested a review from a team February 10, 2018 15:57
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 10, 2018
@BridgeAR
Copy link
Member Author

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@ChALkeR
Copy link
Member

ChALkeR commented Feb 10, 2018

assert.doesNotThrow looks widely used, btw. Not entirely sure how many of those are using the built-in assert lib, though.

@BridgeAR
Copy link
Member Author

@ChALkeR I am also pretty sure people use it. The question is though if we want to encourage new people to use a pretty useless API that should be replaced with comments.

And because it is used right now, I think a doc only deprecation is best in this case.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 10, 2018

@BridgeAR Note that chai implements this differently: http://chaijs.com/api/assert/#method_doesnotthrow

tape seems to roughly match Node.js impl: https://github.com/substack/tape/blob/master/lib/test.js#L515

@BridgeAR
Copy link
Member Author

@ChALkeR yes, the chai implementation has a use case. Even though I am not not sold on testing explicitly against a type of error / message and accept all others.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

I'm -1 to deprecating. Eliminating this from our tests is fine. If people want to use this API for whatever reason, that's their choice.

@ChALkeR
Copy link
Member

ChALkeR commented Feb 10, 2018

Even without deprecating, we could add a note to assert.doesNotThrow() documentation that it's not very useful and is there for consistency.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 10, 2018

@ChALkeR I like that idea.

@@ -316,13 +316,19 @@ parameter is an instance of an [`Error`][] then it will be thrown instead of the
<!-- YAML
added: v0.1.21
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/REPLAEME
Copy link
Member

Choose a reason for hiding this comment

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

Nit: typo REPLAEME

@mcollina
Copy link
Member

@cjihrig idea is also ok. Can you send an alternate PR @BridgeAR?

@BridgeAR
Copy link
Member Author

@cjihrig

If people want to use this API for whatever reason, that's their choice.

That holds true even after this documentation only deprecation. There will not be any warning what so ever. But people who try to find out how the assert module works will clearly see that this is actually not that useful. Only adding a regular comment does not solve the same as well because it is not that obvious.

I tried to find a single use case but I could not find any. Not with any argument combination. That is why I suggest to doc-only deprecate the API.

The chai implementation works differently and even though I do not know a good reason for that API either, I think that it works the way I would have expected it to work here. But changing it to that implementation is not possible as that would be a hard breaking change.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 11, 2018

That holds true even after this documentation only deprecation. There will not be any warning what so ever.

Right, but a docs deprecation sort of means we're planning to runtime deprecate / remove it one day.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 12, 2018

Right, but a docs deprecation sort of means we're planning to runtime deprecate / remove it one day.

I am not aware that this is written down anywhere. I do not see that as a necessary subsequent step. I personally would not want to remove this API (probably ever) because it is used too often. A runtime deprecation is probably also not in place for multiple more years out of my perspective. That might be evaluated again at some point of time but even then: to change the deprecation from documentation-only to something else someone has to open a PR for it and it would probably be denied.

Therefore I would still like to land this as is.

@BridgeAR BridgeAR requested a review from a team February 13, 2018 13:12
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

I'm good with this as a docs-only deprecation does not require that we have to upgrade to to a runtime deprecation later. Although, I'd also be ok with just a docs comment also.

@BridgeAR
Copy link
Member Author

Ping @cjihrig

@cjihrig
Copy link
Contributor

cjihrig commented Feb 16, 2018

I'm still -1 on an official docs deprecation, but am fine with something like @ChALkeR suggested.

@BridgeAR
Copy link
Member Author

@cjihrig as far as I understand your reasons you were mainly against this ever becoming a Runtime deprecation, right? Because I believe there should be other ways to guarantee that than not deprecating this at all.

@BridgeAR BridgeAR requested a review from a team February 16, 2018 21:20
@BridgeAR BridgeAR added tsc-review tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Feb 22, 2018
@@ -330,6 +330,11 @@ changes:
Asserts that the function `block` does not throw an error. See
[`assert.throws()`][] for more details.

Please note: Using `assert.doesNotThrow()` is not recommended because there is
no benefit by catching an error and then rethrowing it. Instead, consider adding
Copy link
Member

Choose a reason for hiding this comment

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

Nit: by -> to

Copy link
Member

@ChALkeR ChALkeR Mar 5, 2018

Choose a reason for hiding this comment

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

s/recommended/actually useful/

Given that we are not deprecating it and that there is no direct harm, I don't see a reason to recommend avoiding it. Comment vs assert.doesNotThrow is pure stylistic.

A note that it doesn't do anything useful and could be replaced with a comment seems better fit, imo.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can do both. Say that it is not useful and therefore not recommended to use.

@addaleax
Copy link
Member

addaleax commented Mar 5, 2018

LGTM with @ChALkeR’s comment

Edit, to maybe clarify a bit: API reference documentation is supposed to be more informative than instructive. It’s the same reason we avoid “you” in our documentation.

@gibfahn
Copy link
Member

gibfahn commented Mar 5, 2018

Edit, to maybe clarify a bit: API reference documentation is supposed to be more informative than instructive. It’s the same reason we avoid “you” in our documentation.

Why? I've never really understood this.

If you pick a random page from the std library of another language, they all seem to use "you" indiscriminately:

Fundamentally we're instructing users of Node in how we think they should use the APIs we provide. Making the tone more formal (e.g. making everything passive) doesn't really change that.

@Trott Trott dismissed their stale review March 5, 2018 20:03

concern has been addressed

Please note: Using `assert.doesNotThrow()` is not recommended because there is
no benefit by catching an error and then rethrowing it. Instead, consider adding
a comment next to the specific code path that should not throw and keep all
error messages in your code as expressive as possible.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: your :-(

Perhaps

Instead, consider adding a comment next to the specific code path that should
not throw and keep add error messages as expressive as possible.

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell There's an error in your proposed text. I think the word add should be deleted?

@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 6, 2018

Comments addressed.

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite/232/

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

BridgeAR commented Mar 8, 2018

Ping @jasnell @addaleax

Please note: Using `assert.doesNotThrow()` is actually not useful because there
is no benefit by catching an error and then rethrowing it. Instead, consider
adding a comment next to the specific code path that should not throw and keep
error messages as expressive as possible.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested rewording ...

The `assert.doesNotThrow()` method is not as useful in practice as it may
first appear. Consider, instead, adding comments to the specific code paths
that should not throw errors and keep error messages as expressive as possible.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 11, 2018
Using `assert.doesNotThrow()` has no benefit over adding a comment
next to some code that should not throw. Therefore it is from now
on discouraged to use it.

PR-URL: nodejs#18699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 8b69d4a

@BridgeAR BridgeAR closed this Mar 11, 2018
@gibfahn gibfahn changed the title doc: doc-only deprecate assert.doesNotThrow() doc: add warning to assert.doesNotThrow() Mar 11, 2018
targos pushed a commit that referenced this pull request Mar 17, 2018
Using `assert.doesNotThrow()` has no benefit over adding a comment
next to some code that should not throw. Therefore it is from now
on discouraged to use it.

PR-URL: #18699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@targos targos mentioned this pull request Mar 18, 2018
@BridgeAR BridgeAR removed the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 19, 2018
@BridgeAR
Copy link
Member Author

I removed the semver-minor label as that does not apply anymore since it is only a comment.

MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
Using `assert.doesNotThrow()` has no benefit over adding a comment
next to some code that should not throw. Therefore it is from now
on discouraged to use it.

PR-URL: #18699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Using `assert.doesNotThrow()` has no benefit over adding a comment
next to some code that should not throw. Therefore it is from now
on discouraged to use it.

PR-URL: nodejs#18699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018
Using `assert.doesNotThrow()` has no benefit over adding a comment
next to some code that should not throw. Therefore it is from now
on discouraged to use it.

PR-URL: nodejs#18699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 6, 2018
Using `assert.doesNotThrow()` has no benefit over adding a comment
next to some code that should not throw. Therefore it is from now
on discouraged to use it.

Backport-PR-URL: #22380
PR-URL: #18699
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 6, 2018
@BridgeAR BridgeAR deleted the deprecate-does-not-throw branch April 1, 2019 23:39
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.