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.

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

tools: alphabetize ESLint rules #19100

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 2, 2018

I have never found it useful that the ESLint rules (in the
configuration file) are organized by the categorization the rules are
given in the ESLint documentation. It has only been a source of
annoyance as one has to look up where the rule is in the docs to figure
out where it should go in the configuration file.

This change re-orders the rules so they are in alphabetical order.
Simple!

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

@Trott Trott added the tools Issues and PRs related to the tools directory. label Mar 2, 2018
@richardlau
Copy link
Member

cc @nodejs/linting

'dot-location': ['error', 'property'],
'dot-notation': 'error',
'eol-last': 'error',
eqeqeq: ['error', 'smart'],
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason some of these are ' quoted and others not? If not, can we pick one style and be consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I definitely prefer it be consistent and intend to introduce a lint rule requiring that (if someone doesn't beat me to it). Was going to wait for this to land first to keep the pull requests minimal and focused. By waiting for this to land first, we avoid a merge conflict.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing they're quoted as needed for object properties.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I don't like that and would prefer that they all be quoted for consistency if one needs to be quoted. But that's just a preference. My plan is to try to figure out what the most common practice is in the code base, and then enforce that. If it's what's going on here, then so be it.

@richardlau
Copy link
Member

categories assigned to them on the

Editing error? (PR description and commit message).

@Trott
Copy link
Member Author

Trott commented Mar 3, 2018

Editing error? (PR description and commit message).

Yup. Fixing right now. Thanks.

I have never found it useful that the ESLint rules (in the
configuration file) are organized by the categorization the rules are
given in the ESLint documentation. It has only been a source of
annoyance as one has to look up where the rule is in the docs to figure
out where it should go in the configuration file.

This change re-orders the rules so they are in alphabetical order.
Simple!
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

It has only been a source of
annoyance as one has to look up where the rule is in the docs to figure
out where it should go in the configuration file.

Agreed!

'dot-location': ['error', 'property'],
'dot-notation': 'error',
'eol-last': 'error',
eqeqeq: ['error', 'smart'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing they're quoted as needed for object properties.

@Trott
Copy link
Member Author

Trott commented Mar 3, 2018

@Trott
Copy link
Member Author

Trott commented Mar 3, 2018

(CI failures appear build-related and not related to this change...I mean, that's probably obvious. If the linter passed, I think we're good here.)

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2018
@Trott
Copy link
Member Author

Trott commented Mar 5, 2018

Landed in c221355

@Trott Trott closed this Mar 5, 2018
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 5, 2018
Trott added a commit that referenced this pull request Mar 5, 2018
I have never found it useful that the ESLint rules (in the
configuration file) are organized by the categorization the rules are
given in the ESLint documentation. It has only been a source of
annoyance as one has to look up where the rule is in the docs to figure
out where it should go in the configuration file.

This change re-orders the rules so they are in alphabetical order.
Simple!

PR-URL: #19100
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Mar 5, 2018
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Trott added a commit to Trott/io.js that referenced this pull request Mar 8, 2018
Refs: nodejs#19100 (comment)
PR-URL: nodejs#19156
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
I have never found it useful that the ESLint rules (in the
configuration file) are organized by the categorization the rules are
given in the ESLint documentation. It has only been a source of
annoyance as one has to look up where the rule is in the docs to figure
out where it should go in the configuration file.

This change re-orders the rules so they are in alphabetical order.
Simple!

PR-URL: nodejs#19100
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Refs: nodejs#19100 (comment)
PR-URL: nodejs#19156
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@Trott Trott deleted the reorder-eslint-props branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.