doc: add warning to assert.doesNotThrow() #18699

BridgeAR · 2018-02-10T15:57:28Z

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 · 2018-02-10T15:58:00Z

CI https://ci.nodejs.org/job/node-test-commit-light/251/

mcollina

LGTM

ChALkeR · 2018-02-10T19:21:43Z

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

BridgeAR · 2018-02-10T19:30:39Z

@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 · 2018-02-10T19:36:07Z

@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 · 2018-02-10T19:49:38Z

@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.

cjihrig

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 · 2018-02-10T22:25:48Z

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

cjihrig · 2018-02-10T22:26:51Z

@ChALkeR I like that idea.

lpinca · 2018-02-11T08:05:43Z

doc/api/assert.md

@@ -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


Nit: typo REPLAEME

mcollina · 2018-02-11T08:12:18Z

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

BridgeAR · 2018-02-11T16:38:47Z

@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 · 2018-02-11T19:37:23Z

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 · 2018-02-12T09:59:06Z

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.

jasnell

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 · 2018-02-16T15:11:09Z

Ping @cjihrig

cjihrig · 2018-02-16T15:15:20Z

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

BridgeAR · 2018-02-16T15:24:50Z

@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.

gibfahn · 2018-03-05T15:02:32Z

doc/api/assert.md

@@ -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


Nit: by -> to

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.

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

addaleax · 2018-03-05T17:33:16Z

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 · 2018-03-05T17:49:43Z

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.

concern has been addressed

jasnell · 2018-03-06T15:43:18Z

doc/api/assert.md

+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.


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.

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

BridgeAR · 2018-03-06T23:25:17Z

Comments addressed.

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

BridgeAR · 2018-03-08T11:33:19Z

Ping @jasnell @addaleax

jasnell · 2018-03-08T17:02:25Z

doc/api/assert.md

+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.


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.

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

BridgeAR · 2018-03-11T01:48:07Z

Landed in 8b69d4a

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

BridgeAR · 2018-03-19T10:56:02Z

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

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

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 requested a review from a team February 10, 2018 15:57

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 10, 2018

BridgeAR mentioned this pull request Feb 10, 2018

assert: adds rejects() and doesNotReject() #18023

Closed

4 tasks

daynin approved these changes Feb 10, 2018

View reviewed changes

mcollina approved these changes Feb 10, 2018

View reviewed changes

cjihrig previously requested changes Feb 10, 2018

View reviewed changes

lpinca approved these changes Feb 11, 2018

View reviewed changes

BridgeAR requested a review from a team February 13, 2018 13:12

jasnell approved these changes Feb 13, 2018

View reviewed changes

gibfahn approved these changes Feb 16, 2018

View reviewed changes

BridgeAR requested a review from a team February 16, 2018 21:20

BridgeAR added tsc-review tsc-agenda Issues and PRs to discuss during the meetings of the TSC. labels Feb 22, 2018

gibfahn approved these changes Mar 5, 2018

View reviewed changes

targos approved these changes Mar 5, 2018

View reviewed changes

thefourtheye approved these changes Mar 6, 2018

View reviewed changes

jasnell requested changes Mar 6, 2018

View reviewed changes

fixup

697c2e3

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

gibfahn mentioned this pull request Mar 6, 2018

doc: remove personal pronoun guidance from style guide #19186

Closed

2 tasks

jasnell approved these changes Mar 8, 2018

View reviewed changes

BridgeAR closed this Mar 11, 2018

gibfahn changed the title ~~doc: doc-only deprecate assert.doesNotThrow()~~ doc: add warning to assert.doesNotThrow() Mar 11, 2018

gibfahn added lts-watch-v6.x labels Mar 11, 2018

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

BridgeAR removed the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 19, 2018

MylesBorins mentioned this pull request Sep 6, 2018

v8.12.0 proposal #21593

Merged

BridgeAR deleted the deprecate-does-not-throw branch April 1, 2019 23:39

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add warning to assert.doesNotThrow() #18699

doc: add warning to assert.doesNotThrow() #18699

BridgeAR commented Feb 10, 2018

BridgeAR commented Feb 10, 2018

mcollina left a comment

ChALkeR commented Feb 10, 2018 •

edited

Loading

BridgeAR commented Feb 10, 2018

ChALkeR commented Feb 10, 2018 •

edited

Loading

BridgeAR commented Feb 10, 2018

cjihrig left a comment

ChALkeR commented Feb 10, 2018 •

edited

Loading

cjihrig commented Feb 10, 2018

lpinca Feb 11, 2018

mcollina commented Feb 11, 2018

BridgeAR commented Feb 11, 2018

cjihrig commented Feb 11, 2018

BridgeAR commented Feb 12, 2018 •

edited

Loading

jasnell left a comment

BridgeAR commented Feb 16, 2018

cjihrig commented Feb 16, 2018

BridgeAR commented Feb 16, 2018

gibfahn Mar 5, 2018

ChALkeR Mar 5, 2018 •

edited

Loading

BridgeAR Mar 5, 2018

addaleax commented Mar 5, 2018 •

edited

Loading

gibfahn commented Mar 5, 2018

jasnell Mar 6, 2018

Trott Mar 6, 2018

BridgeAR commented Mar 6, 2018

BridgeAR commented Mar 8, 2018

jasnell Mar 8, 2018

BridgeAR commented Mar 11, 2018

BridgeAR commented Mar 19, 2018

doc: add warning to assert.doesNotThrow() #18699

doc: add warning to assert.doesNotThrow() #18699

Conversation

BridgeAR commented Feb 10, 2018

Checklist

Affected core subsystem(s)

BridgeAR commented Feb 10, 2018

mcollina left a comment

Choose a reason for hiding this comment

ChALkeR commented Feb 10, 2018 • edited Loading

BridgeAR commented Feb 10, 2018

ChALkeR commented Feb 10, 2018 • edited Loading

BridgeAR commented Feb 10, 2018

cjihrig left a comment

Choose a reason for hiding this comment

ChALkeR commented Feb 10, 2018 • edited Loading

cjihrig commented Feb 10, 2018

lpinca Feb 11, 2018

Choose a reason for hiding this comment

mcollina commented Feb 11, 2018

BridgeAR commented Feb 11, 2018

cjihrig commented Feb 11, 2018

BridgeAR commented Feb 12, 2018 • edited Loading

jasnell left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 16, 2018

cjihrig commented Feb 16, 2018

BridgeAR commented Feb 16, 2018

gibfahn Mar 5, 2018

Choose a reason for hiding this comment

ChALkeR Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Mar 5, 2018

Choose a reason for hiding this comment

addaleax commented Mar 5, 2018 • edited Loading

gibfahn commented Mar 5, 2018

jasnell Mar 6, 2018

Choose a reason for hiding this comment

Trott Mar 6, 2018

Choose a reason for hiding this comment

BridgeAR commented Mar 6, 2018

BridgeAR commented Mar 8, 2018

jasnell Mar 8, 2018

Choose a reason for hiding this comment

BridgeAR commented Mar 11, 2018

BridgeAR commented Mar 19, 2018

ChALkeR commented Feb 10, 2018 •

edited

Loading

ChALkeR commented Feb 10, 2018 •

edited

Loading

ChALkeR commented Feb 10, 2018 •

edited

Loading

BridgeAR commented Feb 12, 2018 •

edited

Loading

ChALkeR Mar 5, 2018 •

edited

Loading

addaleax commented Mar 5, 2018 •

edited

Loading