FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Alphabetize error list by maclover7 · Pull Request #15083 · nodejs/node · GitHub

/ node Public

Alphabetize error list - #15083

Closed
maclover7 wants to merge 2 commits into
nodejs:masterfrom
maclover7:jm-alphabetize-errors
ConversationCommits (2)ChecksFiles changed
Closed

Alphabetize error list#15083
maclover7 wants to merge 2 commits into
nodejs:masterfrom
maclover7:jm-alphabetize-errors

Conversation

maclover7 commented Aug 30, 2017

Copy link
Copy Markdown
Contributor

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

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

jasnell 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Woo! Linting!

mscdex commented Aug 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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 approved these changes Aug 30, 2017
View reviewed changes

refack 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

eslint FTW 🥇

refack reviewed Aug 30, 2017
View reviewed changes
Comment thread lib/.eslintrc.yaml Outdated

refack Aug 30, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

/* esline-enable alphabetize-errors */

in lib/internal/errors.js

refack commented Aug 30, 2017

Copy link
Copy Markdown
Contributor

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.

refack self-assigned this Aug 30, 2017

maclover7 commented Aug 30, 2017

Copy link
Copy Markdown
Contributor Author

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 reviewed Aug 31, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

alexwhitman Aug 31, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should be eslint-enable, not esline.

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 commented Sep 1, 2017

Copy link
Copy Markdown
Contributor Author

Good catch, updated 👍

BridgeAR approved these changes Sep 1, 2017
View reviewed changes

BridgeAR 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Still LGTM

jasnell commented Sep 1, 2017

Copy link
Copy Markdown
Member

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

refack commented Sep 1, 2017

Copy link
Copy Markdown
Contributor

Landed in 324aa64 b12d779

refack closed this Sep 1, 2017
refack pushed a commit that referenced this pull request Sep 1, 2017
errors: alphabetize error codes
324aa64 
PR-URL: #15083
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
refack pushed a commit that referenced this pull request Sep 1, 2017
errors,tools: alphabetize-errors lint rule
b12d779 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
maclover7 deleted the jm-alphabetize-errors branch September 1, 2017 22:25

Trott commented Sep 2, 2017

Copy link
Copy Markdown
Member

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

addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
errors: alphabetize error codes
23a6e8f 
PR-URL: nodejs/node#15083
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
errors,tools: alphabetize-errors lint rule
519cda8 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins commented Sep 10, 2017

Copy link
Copy Markdown
Contributor

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

jasnell pushed a commit that referenced this pull request Sep 25, 2017
errors,tools: alphabetize-errors lint rule
2e03138 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins commented Sep 29, 2017

Copy link
Copy Markdown
Contributor

ping re backport

Trott commented Sep 29, 2017

Copy link
Copy Markdown
Member

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 commented Sep 29, 2017

Copy link
Copy Markdown
Member

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

Trott pushed a commit to Trott/io.js that referenced this pull request Sep 29, 2017
errors,tools: alphabetize-errors lint rule
87de4eb 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Trott commented Sep 29, 2017

Copy link
Copy Markdown
Member

@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 commented Sep 29, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

refack commented Sep 29, 2017

Copy link
Copy Markdown
Contributor

I'll fix that with a backport of #15578

MylesBorins commented Sep 29, 2017

Copy link
Copy Markdown
Contributor

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 commented Sep 29, 2017

Copy link
Copy Markdown
Member

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

MylesBorins mentioned this pull request Sep 29, 2017
Closed
2 tasks
not-an-aardvark mentioned this pull request Oct 29, 2017
Closed
3 tasks
jasnell mentioned this pull request Oct 30, 2017
Merged
refack removed their assignment Oct 12, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
+3 more reviewers
refack refack approved these changes
alexwhitman alexwhitman left review comments
danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated in Node.js core. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL