Skip to content

process: improve hrtime() error message - #14324

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:deawkwardize
Closed

process: improve hrtime() error message#14324
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:deawkwardize

Conversation

@Trott

@TrottTrott commented Jul 17, 2017

Copy link
Copy Markdown
Member

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

@TrottTrott 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-botnodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jul 17, 2017
@TrottTrott 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
Copy Markdown
MemberAuthor

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
Copy Markdown
Member

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

@Fishrock123

Copy link
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

Trott commented Jul 18, 2017

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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

@Trott

Copy link
Copy Markdown
MemberAuthor

Comment threadlib/internal/errors.js Outdated

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
MemberAuthor

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.

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@evanlucasevanlucas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

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
Copy Markdown
MemberAuthor

@Trott

Copy link
Copy Markdown
MemberAuthor

Landed in 43e105f

@TrottTrott closed this Jul 20, 2017
Trott added a commit to Trott/io.js that referenced this pull request Jul 20, 2017
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>
@Trott
Trott deleted the deawkwardize branch January 13, 2022 22:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

errorsIssues and PRs related to JavaScript errors originated in Node.js core.processIssues and PRs related to the process subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@Trott@lpinca@Fishrock123@mcollina@refack@jasnell@evanlucas@cjihrig@gibfahn@nodejs-github-bot