doc: make error descriptions more concise by Trott · Pull Request #16954 · nodejs/node · GitHub

Remove the practice of starting most error descriptions with "Used when"
or wordier variations.

Change errors of the form:

Used when the type of an asynchronous resource is invalid.

...to:

The type of an asynchronous resource was invalid.

Change errors of the form:

The 'ERR_INVALID_CURSOR_POS' is thrown specifically when a cursor on
a given stream is attempted to move to a specified row without a specified
column.

...to:

A cursor on a given stream cannot be moved to a specified row without
a specified column.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

errors doc

left a few comments, some are just adding a comma here or there, feel free to treat as nits :)

could --> could not?

Done. I also removed properly as it seems superfluous.

comma after disable FIPS mode?

done, thanks

I know it wasn't there originally, but should HTTP be changes to HTTP/2?

It would seem so. Done.

comma after by the client?

done, thanks

LGTM. FWIW, once the error conversion is complete, I'm planning a sprint of changes to fill in the details on these errors in the docs, including example code (where possible) showing how to trigger the error and an explanation on how to fix it.

Making @maclover7 's nit explicit - otherwise looks like a nice positive change

@benjamingr I've addressed all of @maclover7's nits and also edited a few more messages that landed since I originally opened this. PTAL. (I left these changes in separate commits for easier reviewing if you don't want to read everything all over again.)

LGTM

@Trott just in case you haven't noticed I've approved those changes (after your commit but before your comment).

CI: https://ci.nodejs.org/job/node-test-pull-request/11435/

Left a bunch of suggestions...for better consistency 😅

There is an extra e in receivee

s/Streams/streams?

s/contains/contained/?

The previous descriptions are using an attempt was made to..., we may want to be consistent here.

Extra space at the end.

For consistency, An attempt was made to enable...but FIPS mode was not available.

s/is/was/

Maybe drop the when?

I think I'll go with if instead.

s/is/was/

Remove the practice of starting most error descriptions with "Used when" or wordier variations. Change errors of the form: > Used when the type of an asynchronous resource is invalid. ...to: > The type of an asynchronous resource was invalid. Change errors of the form: > The `'ERR_INVALID_CURSOR_POS'` is thrown specifically when a cursor on > a given stream is attempted to move to a specified row without a specified > column. ...to: > A cursor on a given stream cannot be moved to a specified row without > a specified column.

Addressed almost all of the awesome nits from @joyeecheung. PTAL

Thanks for bearing with me :D

CI: https://ci.nodejs.org/job/node-test-pull-request/11488/

Remove the practice of starting most error descriptions with "Used when" or wordier variations. Change errors of the form: > Used when the type of an asynchronous resource is invalid. ...to: > The type of an asynchronous resource was invalid. Change errors of the form: > The `'ERR_INVALID_CURSOR_POS'` is thrown specifically when a cursor on > a given stream is attempted to move to a specified row without a > specified column. ...to: > A cursor on a given stream cannot be moved to a specified row without > a specified column. PR-URL: nodejs#16954 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Landed in 959c425.

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Remove the practice of starting most error descriptions with "Used when" or wordier variations. Change errors of the form: > Used when the type of an asynchronous resource is invalid. ...to: > The type of an asynchronous resource was invalid. Change errors of the form: > The `'ERR_INVALID_CURSOR_POS'` is thrown specifically when a cursor on > a given stream is attempted to move to a specified row without a > specified column. ...to: > A cursor on a given stream cannot be moved to a specified row without > a specified column. PR-URL: nodejs#16954 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

@MylesBorins Backport is in #17622

Remove the practice of starting most error descriptions with "Used when" or wordier variations. Change errors of the form: > Used when the type of an asynchronous resource is invalid. ...to: > The type of an asynchronous resource was invalid. Change errors of the form: > The `'ERR_INVALID_CURSOR_POS'` is thrown specifically when a cursor on > a given stream is attempted to move to a specified row without a > specified column. ...to: > A cursor on a given stream cannot be moved to a specified row without > a specified column. Backport-PR-URL: #17622 PR-URL: #16954 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

@Trott backports for v6.x and v8.x? Feel free to mark don't land if it's a hassle.

@Trott backports for v6.x and v8.x? Feel free to mark don't land if it's a hassle.

Node.js error codes don't exist in 6.x so marking as dont-land for that version.

They exist in 8.x so I'll take a closer look at that...

Remove the practice of starting most error descriptions with "Used when" or wordier variations. Change errors of the form: > Used when the type of an asynchronous resource is invalid. ...to: > The type of an asynchronous resource was invalid. Change errors of the form: > The `'ERR_INVALID_CURSOR_POS'` is thrown specifically when a cursor on > a given stream is attempted to move to a specified row without a > specified column. ...to: > A cursor on a given stream cannot be moved to a specified row without > a specified column. PR-URL: nodejs#16954 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

@gibfahn 8.x backport in #17767 and assigned to you.

Remove the practice of starting most error descriptions with "Used when" or wordier variations. Change errors of the form: > Used when the type of an asynchronous resource is invalid. ...to: > The type of an asynchronous resource was invalid. Change errors of the form: > The `'ERR_INVALID_CURSOR_POS'` is thrown specifically when a cursor on > a given stream is attempted to move to a specified row without a > specified column. ...to: > A cursor on a given stream cannot be moved to a specified row without > a specified column. PR-URL: #16954 Backport-PR-URL: #17767 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Trott added doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Nov 11, 2017

nodejs-github-bot added doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Nov 11, 2017

maclover7 reviewed Nov 11, 2017

View reviewed changes

jasnell approved these changes Nov 12, 2017

View reviewed changes

gireeshpunathil approved these changes Nov 12, 2017

View reviewed changes

benjamingr requested changes Nov 12, 2017

View reviewed changes

Trott force-pushed the used-when branch from 6e1bab8 to 37b6913 Compare November 12, 2017 14:00

benjamingr approved these changes Nov 12, 2017

View reviewed changes

Trott mentioned this pull request Nov 14, 2017

crypto: Better docs for cases where peer's public key is invalid #16849

Closed

4 tasks

tniessen approved these changes Nov 15, 2017

View reviewed changes

joyeecheung reviewed Nov 15, 2017

View reviewed changes

Trott added 6 commits November 15, 2017 14:38

squash: fix errors that have landed since this opened

b9e372c

squash: cpuUsage nit from maclover7

2edcd00

squash: comma nits from maclover7

5fadfe0

squash: HTTP/2 nit from maclover7

b9fd330

squash: mega-nits from joyee

f85b90d

Trott force-pushed the used-when branch from cb2ece2 to f85b90d Compare November 15, 2017 23:28

joyeecheung approved these changes Nov 16, 2017

View reviewed changes

Trott closed this Nov 17, 2017

MylesBorins added the backport-requested-v9.x label Dec 11, 2017

Trott mentioned this pull request Dec 11, 2017

[v9.x backport] doc: make error descriptions more concise #17622

Closed

4 tasks

Trott removed the backport-requested-v9.x label Dec 11, 2017

MylesBorins added the backported-to-v9.x label Dec 12, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added backport-requested-v6.x labels Dec 19, 2017

Trott added dont-land-on-v6.x and removed backport-requested-v6.x labels Dec 19, 2017

Trott mentioned this pull request Dec 19, 2017

[v8.x backport] doc: make error descriptions more concise #17767

Closed

3 tasks

gibfahn added backported-to-v8.x and removed backport-requested-v8.x labels Dec 20, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

Trott deleted the used-when branch January 13, 2022 22:47

doc: make error descriptions more concise by Trott · Pull Request #16954 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: make error descriptions more concise - #16954

doc: make error descriptions more concise#16954Trott wants to merge 6 commits intonodejs:masternodejs/node:masterfrom Trott:used-whenTrott/io.js:used-whenCopy head branch name to clipboard

Conversation

Trott commented Nov 11, 2017

Checklist

Affected core subsystem(s)

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 12, 2017

Uh oh!

maclover7 commented Nov 12, 2017

Uh oh!

benjamingr commented Nov 12, 2017

Uh oh!

Trott commented Nov 14, 2017

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 15, 2017

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Nov 17, 2017

Uh oh!

Trott commented Nov 17, 2017

Uh oh!

MylesBorins commented Dec 11, 2017

Uh oh!

Trott commented Dec 11, 2017

doc: make error descriptions more concise#16954
Trott wants to merge 6 commits into
nodejs:masterfrom
Trott:used-when