Alphabetize error list #15083

maclover7 · 2017-08-30T02:24:06Z

Includes a new ESLint rule to make sure the list doesn't become un-alphabetized again :)

Checklist

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

Affected core subsystem(s)

errors, tools

jasnell

Woo! Linting!

mscdex · 2017-08-30T04:18:20Z

Shouldn't the linting be explicitly limited to lib/internal/errors.js to reduce the chance of it accidentally matching something else that starts with a capital 'E'?

benjamingr · 2017-08-30T11:28:37Z

I haven't maintained that code - but are we sure the overhead of maintaining a plugin is worth it? It sounds like a lot more overhead than having non-alphabetized errors.

refack

eslint FTW 🥇

refack · 2017-08-30T11:33:33Z

lib/.eslintrc.yaml

@@ -3,3 +3,4 @@ rules:
  require-buffer: error
  buffer-constructor: error
  no-let-in-for-declaration: error
+  alphabetize-errors: error


Per @mscdex's comment It might be best to replace this with a:

/* esline-enable alphabetize-errors */

in lib/internal/errors.js

refack · 2017-08-30T11:36:32Z

I haven't maintained that code - but are we sure the overhead of maintaining a plugin is worth it? It sounds like a lot more overhead than having non-alphabetized errors.

It's been a pain, especially conflict-wise. Also AFAIK this plugin could later be applied to validate that the codes in lib/internal/errors.js and in doc/api/errors.md are in sync.

maclover7 · 2017-08-30T15:37:00Z

Also AFAIK this plugin could later be applied to validate that the codes in lib/internal/errors.js and in doc/api/errors.md are in sync.

Yep, this is something that I want to work on later on.

alexwhitman · 2017-08-31T08:35:25Z

lib/internal/errors.js

@@ -1,3 +1,5 @@
+/* esline-enable alphabetize-errors */


This should be eslint-enable, not esline.

To make sure errors in lib/internal/errors.js (are defined via `E`) will stay in alphabetical order going forward.

maclover7 · 2017-09-01T00:27:31Z

Good catch, updated 👍

BridgeAR

Still LGTM

jasnell · 2017-09-01T17:56:38Z

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

refack · 2017-09-01T21:30:32Z

Landed in 324aa64 b12d779

PR-URL: #15083 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

To make sure errors in lib/internal/errors.js (are defined via `E`) will stay in alphabetical order going forward. PR-URL: #15083 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Trott · 2017-09-02T22:18:25Z

Using a comment to enable a custom lint rule on a single file. Very clever. I salute you. 🎉

PR-URL: nodejs/node#15083 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

To make sure errors in lib/internal/errors.js (are defined via `E`) will stay in alphabetical order going forward. PR-URL: nodejs/node#15083 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2017-09-10T02:20:56Z

This will need to be backported to v8.x

I attempted to use the lint rule to find out of order error codes, but unfortunately on the 8.x branch the lint rule wasn't doing anything, same with master. I opened an issue

To make sure errors in lib/internal/errors.js (are defined via `E`) will stay in alphabetical order going forward. PR-URL: #15083 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins · 2017-09-29T20:09:06Z

ping re backport

Trott · 2017-09-29T20:15:35Z

If I'm not mistaken, this is dependent on multiple semver-majors and might be considered a 9.x-and-above only? Like, any existing error moved to the new error system is going to be semver-major...

Trott · 2017-09-29T20:20:00Z

Actually, never mind, I have a backport-ish coming shortly.

To make sure errors in lib/internal/errors.js (are defined via `E`) will stay in alphabetical order going forward. PR-URL: nodejs#15083 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Trott · 2017-09-29T20:23:46Z

@MylesBorins Current v8.x-staging has the lint rule properly enabled and it is flagging two errors (that are duplicates) as being out of order. So this doesn't really need to be backported after all I don't think. But you'll probably want to find where that error was introduced and fix it so that there aren't a bunch of broken commits, yeah?

Trott · 2017-09-29T20:26:52Z

First bad commit is 3b6c00a. (make lint-js fails.)

refack · 2017-09-29T20:28:00Z

I'll fix that with a backport of #15578

MylesBorins · 2017-09-29T20:29:04Z

thanks all... I'll rebase it in before the first bad commit so we don't have broken stuff in the tree. Can you please include that information in the PR

Trott · 2017-09-29T20:29:35Z

Thanks @refack and @MylesBorins. Sounds like there's nothing more for me to do here, but if I'm wrong, let me know.

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 Aug 30, 2017

jasnell approved these changes Aug 30, 2017

View reviewed changes

cjihrig approved these changes Aug 30, 2017

View reviewed changes

danbev approved these changes Aug 30, 2017

View reviewed changes

refack approved these changes Aug 30, 2017

View reviewed changes

refack reviewed Aug 30, 2017

View reviewed changes

refack self-assigned this Aug 30, 2017

maclover7 force-pushed the jm-alphabetize-errors branch from 5765c33 to 8a1e6aa Compare August 30, 2017 15:36

BridgeAR approved these changes Aug 31, 2017

View reviewed changes

alexwhitman reviewed Aug 31, 2017

View reviewed changes

maclover7 added 2 commits August 31, 2017 20:26

errors, tools: create alphabetize-errors lint rule

8d0244b

To make sure errors in lib/internal/errors.js (are defined via `E`) will stay in alphabetical order going forward.

errors: alphabetize lib/internal/errors.js

aaec74c

maclover7 force-pushed the jm-alphabetize-errors branch from 8a1e6aa to aaec74c Compare September 1, 2017 00:27

BridgeAR approved these changes Sep 1, 2017

View reviewed changes

refack closed this Sep 1, 2017

maclover7 deleted the jm-alphabetize-errors branch September 1, 2017 22:25

MylesBorins added the backport-requested-v8.x label Sep 10, 2017

MylesBorins mentioned this pull request Sep 29, 2017

Error alphabetization fixes #15307

Closed

2 tasks

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

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

not-an-aardvark mentioned this pull request Oct 29, 2017

test: add tests for eslint rules #16138

Closed

3 tasks

jasnell mentioned this pull request Oct 30, 2017

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

Merged

refack removed their assignment Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alphabetize error list #15083

Alphabetize error list #15083

maclover7 commented Aug 30, 2017

jasnell left a comment

mscdex commented Aug 30, 2017 •

edited

Loading

benjamingr commented Aug 30, 2017

refack left a comment

refack Aug 30, 2017

refack commented Aug 30, 2017

maclover7 commented Aug 30, 2017

alexwhitman Aug 31, 2017

maclover7 commented Sep 1, 2017

BridgeAR left a comment

jasnell commented Sep 1, 2017

refack commented Sep 1, 2017

Trott commented Sep 2, 2017

MylesBorins commented Sep 10, 2017

MylesBorins commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017 •

edited

Loading

refack commented Sep 29, 2017

MylesBorins commented Sep 29, 2017

Trott commented Sep 29, 2017

Alphabetize error list #15083

Alphabetize error list #15083

Conversation

maclover7 commented Aug 30, 2017

Checklist

Affected core subsystem(s)

jasnell left a comment

Choose a reason for hiding this comment

mscdex commented Aug 30, 2017 • edited Loading

benjamingr commented Aug 30, 2017

refack left a comment

Choose a reason for hiding this comment

refack Aug 30, 2017

Choose a reason for hiding this comment

refack commented Aug 30, 2017

maclover7 commented Aug 30, 2017

alexwhitman Aug 31, 2017

Choose a reason for hiding this comment

maclover7 commented Sep 1, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

jasnell commented Sep 1, 2017

refack commented Sep 1, 2017

Trott commented Sep 2, 2017

MylesBorins commented Sep 10, 2017

MylesBorins commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017

Trott commented Sep 29, 2017 • edited Loading

refack commented Sep 29, 2017

MylesBorins commented Sep 29, 2017

Trott commented Sep 29, 2017

mscdex commented Aug 30, 2017 •

edited

Loading

Trott commented Sep 29, 2017 •

edited

Loading