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.

Sign up for GitHub

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

Jump to bottom

process: improve hrtime() error message #14324

Closed
wants to merge 1 commit into from
Closed

process: improve hrtime() error message #14324

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 17, 2017 •
edited
Loading

Change error message from the form this format:

The "time" array must have a length of 2. Received length 0

...to this format:

The array "time" (length 0) must be length 2.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

process errors test

@Trott Trott added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. labels Jul 17, 2017
@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jul 17, 2017
@Trott Trott added semver-major PRs that contain breaking changes and should be released in the next major version. test Issues and PRs related to the tests. labels Jul 17, 2017
@Trott
Copy link
Member Author

Trott commented Jul 17, 2017

Labeled semver-major because the CTC hasn't yet made a decision about whether these sorts of message changes are considered semver-major or not. (Decision expected in the next few days.)

HOWEVER, it does not really matter for this PR either way because the error message that this changes is not in Node.js 8.0 and is itself semver-major. In other words, whether we land this PR or not, the message is going to change in a semver-major fashion in Node.js 9.0

@lpinca
Copy link
Member

lpinca commented Jul 17, 2017

Is there a particular reason apart from reducing the error message length? The original seems ok to me.

@Fishrock123
Copy link
Contributor

Could be more concise, aren't we semver-minor for these now that we have the error codes thing?

Imo not worth the major change for this though, pretty much just the same thing?

@Trott
Copy link
Member Author

Trott commented Jul 18, 2017

Is there a particular reason apart from reducing the error message length? The original seems ok to me.

@lpinca In addition to reducing the error message length, I'm of the opinion that the current message is awkward.

I was originally going to fix that by simply changing The "time" array must have... to The array "time" must have..., but then I figure that if I was in there making a semver-major change anyway, I might as well improve it as much as I can.

@Trott
Copy link
Member Author

Trott commented Jul 18, 2017

Could be more concise, aren't we semver-minor for these now that we have the error codes thing?

@Fishrock123 #13937 especially #13937 (comment)

There are at least 4 CTC members who believe these changes should still be semver-major at this time, and there are at least 2 who believe they should be semver-patch. So there is not consensus. Thus, I labeled this semver-major based on the "if there's any doubt, then it's major" principle.

@Trott
Copy link
Member Author

Trott commented Jul 18, 2017 •
edited
Loading

Imo not worth the major change for this though, pretty much just the same thing?

@Fishrock123 As I wrote above, the semver major that introduces this error message in the first place has not landed. So the "not worth the major change" is not the right way to think about this, as the semver cost is zero. I'm making a (tiny) semver-major change to another semver-major change and they're both going to land in 9.0.

@Trott
Copy link
Member Author

Trott commented Jul 19, 2017

Needs one more @nodejs/ctc approval, so ping!

@Trott
Copy link
Member Author

Trott commented Jul 19, 2017

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

refack
refack reviewed Jul 19, 2017
View reviewed changes
lib/internal/errors.js Outdated
@@ -126,8 +126,7 @@ E('ERR_INVALID_ARRAY_LENGTH',
(name, length, actual) => {
const assert = lazyAssert();
assert.strictEqual(typeof actual, 'number');
return `The "${name}" array must have a length of ${
length}. Received length ${actual}`;
return `The array "${name}" (length ${actual}) must be length ${length}.`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be must have length or must be of length?

Copy link
Member

Choose a reason for hiding this comment

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

+1 to must be of length

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, updated with that text.

mcollina
mcollina approved these changes Jul 19, 2017
View reviewed changes
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

evanlucas
evanlucas approved these changes Jul 19, 2017
View reviewed changes
Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
process: improve hrtime() error message
e42cf74 
Change error message from the form this format:

  The "time" array must have a length of 2. Received length 0

...to this format:

  The array "time" (length 0) must be of length 2.
@Trott
Copy link
Member Author

Trott commented Jul 19, 2017

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

@Trott
Copy link
Member Author

Trott commented Jul 20, 2017

Landed in 43e105f

@Trott Trott closed this Jul 20, 2017
Trott added a commit to Trott/io.js that referenced this pull request Jul 20, 2017
@Trott
process: improve hrtime() error message
43e105f 
Change error message from the form this format:

  The "time" array must have a length of 2. Received length 0

...to this format:

  The array "time" (length 0) must be of length 2.

PR-URL: nodejs#14324
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@Trott Trott deleted the deawkwardize branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@evanlucas evanlucas evanlucas approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Trott @lpinca @Fishrock123 @mcollina @refack @jasnell @evanlucas @cjihrig @gibfahn @nodejs-github-bot