errors,tools: ASCIIbetical instead of alphabetical #15578

refack · 2017-09-23T21:42:05Z

Fixes: #15576

Checklist

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

Affected core subsystem(s)

errors,tools

refack · 2017-09-23T21:42:21Z

/cc @maclover7

lpinca · 2017-09-24T08:41:59Z

tools/eslint-rules/alphabetize-errors.js

+        if (prev >= curr) {
+          context.report({
+            node: node,
+            message: message + ` - pref:${prev} < curr:${curr}`


typo: pref -> prev

refack · 2017-09-24T14:08:55Z

I reworked the error message completely:

d:\code\node-cur\lib\internal\errors.js
  147:1  error  Out of ASCIIbetical order - ERR_HTTP_TRAILER_INVALID >= ERR_HTTP2_CONNECT_AUTHORITY           alphabetize-errors
  147:1  error                                  (95)_                >=         2(50)                         alphabetize-errors
  165:1  error  Out of ASCIIbetical order - ERR_HTTP2_HEADER_SINGLE_VALUE >= ERR_HTTP2_HEADERS_AFTER_RESPOND  alphabetize-errors
  165:1  error                                          (95)_             >=                 S(83)            alphabetize-errors
  262:1  error  Out of ASCIIbetical order - ERR_OUT_OF_RANGE >= ERR_OUTOFMEMORY                               alphabetize-errors
  262:1  error                                 (95)_         >=        O(79)                                  alphabetize-errors

✖ 6 problems (6 errors, 0 warnings)

lpinca · 2017-09-24T14:24:03Z

tools/eslint-rules/alphabetize-errors.js

+          let tmp = ' '.repeat(j) + opStr + ' '.repeat(l - oLen - j);
+          const k2 = k - lm.length + 1;
+          const k3 = k2 + km.length;
+          const message2 = tmp.substr(0, k2) + km + tmp.substr(k3) + lm;


Isn't it easier to just show the index where the two strings differ?

This is not really readable. Especially the variable names are pretty bad. Without checking to closely I am also pretty sure this could probably be simplified a bit so it is better to read afterwards.

Easy? 😤 easy for someone else...
_{yeah I was bored this morning}

But seriously, I think it's a nice and informative feedback, and I don't assume this error should happen too often, so I can take it or leave it..

My point is that I don't care if _ >= O for example. I find it sufficient to know the index where the issue happens (e.g. 7 for ERR_OUT_OF_RANGE and ERR_OUTOFMEMORY) but I understand this is very personal.

maclover7 · 2017-09-24T21:22:14Z

@refack interesting PR! Overall looks good, only concern is that the new "is there a problem" / message generating logic is a little bit hard to grok -- would it be possible to move these back to separate functions?

refack · 2017-09-24T21:26:19Z

message generating logic is a little bit hard to grok

Yeah a little bit overboard. Not sure if I could understand it in a week...

refack · 2017-09-25T19:37:52Z

Removed obfus code. Now the errors looks like:

d:\code\node-cur\lib\internal\errors.js
  147:1  error  Out of ASCIIbetical order - ERR_HTTP_TRAILER_INVALID >= ERR_HTTP2_CONNECT_AUTHORITY           alphabetize-errors
  165:1  error  Out of ASCIIbetical order - ERR_HTTP2_HEADER_SINGLE_VALUE >= ERR_HTTP2_HEADERS_AFTER_RESPOND  alphabetize-errors
  262:1  error  Out of ASCIIbetical order - ERR_OUT_OF_RANGE >= ERR_OUTOFMEMORY                               alphabetize-errors

✖ 3 problems (3 errors, 0 warnings)

and IMHO it's informative enough.

Linter CI: https://ci.nodejs.org/job/node-test-linter/12025/

maclover7 · 2017-09-25T20:29:34Z

I'm not a collaborator, but 👍, much simpler now

jasnell

I see the point but I'm not a fan of landing this right now. Perhaps after we get all of the codes migrated, otherwise it just ends up causing additional churn.

refack · 2017-09-26T20:20:25Z

I see the point but I'm not a fan of landing this right now. Perhaps after we get all of the codes migrated, otherwise it just ends up causing additional churn.

Other way around. It's already in and has been breaking the lint CI for over a week. I had to hack the lint machine to use LC_ALL=en_US.

refack · 2017-09-28T01:20:55Z

@jasnell linter fails when it runs on the other (unpatched) machine test-rackspace-freebsd10-x64-1 https://ci.nodejs.org/job/node-test-linter/12073/

jasnell · 2017-09-28T12:56:28Z

hmm... ok. I'd just generally prefer that we not shuffle these ids around too much more until we get them all migrated as it makes getting those PRs landed much more difficult.

Removing my -1

refack · 2017-09-28T14:13:14Z

@nodejs/release you might want to cherry-pick to v8.x-staging (https://ci.nodejs.org/job/node-test-linter/12101/)

not ok 3 - /usr/home/iojs/build/workspace/node-test-linter/lib/internal/errors.js
  ---
  message: Errors in lib/internal/errors.js must be alphabetized
  severity: error
  data:
    line: 244
    column: 1
    ruleId: alphabetize-errors
  messages:
    - message: Errors in lib/internal/errors.js must be alphabetized
      severity: error
      data:
        line: 251
        column: 1
        ruleId: alphabetize-errors
  ...

PR-URL: nodejs/node#15578 Fixes: nodejs/node#15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: nodejs#15578 Fixes: nodejs#15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #15578 Fixes: #15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

MylesBorins · 2017-10-03T21:07:21Z

I lied... need it one more time

please open a new PR

PR-URL: #15578 Fixes: #15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. tools Issues and PRs related to the tools directory. labels Sep 23, 2017

refack force-pushed the reorder-errorcodes branch from 02efe97 to 394d6b7 Compare September 23, 2017 21:57

lpinca reviewed Sep 24, 2017

View reviewed changes

lpinca approved these changes Sep 24, 2017

View reviewed changes

lpinca reviewed Sep 24, 2017

View reviewed changes

refack force-pushed the reorder-errorcodes branch from 443192c to e9f140d Compare September 25, 2017 19:37

jasnell previously requested changes Sep 26, 2017

View reviewed changes

refack closed this Sep 28, 2017

refack force-pushed the reorder-errorcodes branch from e9f140d to 5f46944 Compare September 28, 2017 13:17

refack merged commit 5f46944 into nodejs:master Sep 28, 2017

refack deleted the reorder-errorcodes branch September 28, 2017 13:18

MylesBorins added backported-to-v8.x and removed backported-to-v8.x labels Sep 29, 2017

refack mentioned this pull request Sep 29, 2017

Alphabetize error list #15083

Closed

2 tasks

refack mentioned this pull request Sep 29, 2017

[v8.x backport] errors,tools: ASCIIbetical instead of alphabetical #15689

Closed

3 tasks

addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017

errors,tools: ASCIIbetical instead of alphabetical

ee647fc

PR-URL: nodejs/node#15578 Fixes: nodejs/node#15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

refack added a commit to refack/node that referenced this pull request Oct 3, 2017

errors,tools: ASCIIbetical instead of alphabetical

0e84007

PR-URL: nodejs#15578 Fixes: nodejs#15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

MylesBorins pushed a commit that referenced this pull request Oct 3, 2017

errors,tools: ASCIIbetical instead of alphabetical

7a74504

PR-URL: #15578 Fixes: #15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

MylesBorins added backported-to-v8.x and removed backport-requested-v8.x labels Oct 7, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

MylesBorins pushed a commit that referenced this pull request Dec 19, 2017

errors,tools: ASCIIbetical instead of alphabetical

f63acdb

PR-URL: #15578 Fixes: #15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn pushed a commit that referenced this pull request Dec 20, 2017

errors,tools: ASCIIbetical instead of alphabetical

20b40d1

PR-URL: #15578 Fixes: #15576 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors,tools: ASCIIbetical instead of alphabetical #15578

errors,tools: ASCIIbetical instead of alphabetical #15578

refack commented Sep 23, 2017

refack commented Sep 23, 2017

lpinca Sep 24, 2017

refack commented Sep 24, 2017

lpinca Sep 24, 2017

BridgeAR Sep 24, 2017

refack Sep 24, 2017

lpinca Sep 25, 2017

refack Sep 25, 2017

maclover7 commented Sep 24, 2017

refack commented Sep 24, 2017

refack commented Sep 25, 2017

maclover7 commented Sep 25, 2017

jasnell left a comment

refack commented Sep 26, 2017

refack commented Sep 28, 2017

jasnell commented Sep 28, 2017

refack commented Sep 28, 2017

MylesBorins commented Oct 3, 2017 •

edited

Loading

errors,tools: ASCIIbetical instead of alphabetical #15578

errors,tools: ASCIIbetical instead of alphabetical #15578

Conversation

refack commented Sep 23, 2017

Checklist

Affected core subsystem(s)

refack commented Sep 23, 2017

lpinca Sep 24, 2017

Choose a reason for hiding this comment

refack commented Sep 24, 2017

lpinca Sep 24, 2017

Choose a reason for hiding this comment

BridgeAR Sep 24, 2017

Choose a reason for hiding this comment

refack Sep 24, 2017

Choose a reason for hiding this comment

lpinca Sep 25, 2017

Choose a reason for hiding this comment

refack Sep 25, 2017

Choose a reason for hiding this comment

maclover7 commented Sep 24, 2017

refack commented Sep 24, 2017

refack commented Sep 25, 2017

maclover7 commented Sep 25, 2017

jasnell left a comment

Choose a reason for hiding this comment

refack commented Sep 26, 2017

refack commented Sep 28, 2017

jasnell commented Sep 28, 2017

refack commented Sep 28, 2017

MylesBorins commented Oct 3, 2017 • edited Loading

MylesBorins commented Oct 3, 2017 •

edited

Loading