tools: add sort-requires rule to enforce {a, b, c} = require() is in alphabetical order#44116
Closed
cola119 wants to merge 2 commits intonodejs:mainfrom
Closed
tools: add sort-requires rule to enforce {a, b, c} = require() is in alphabetical order#44116cola119 wants to merge 2 commits intonodejs:mainfrom
sort-requires rule to enforce {a, b, c} = require() is in alphabetical order#44116cola119 wants to merge 2 commits intonodejs:mainfrom
Conversation
nodejs-github-bot
added
needs-ci
cola119
changed the title
require-order rule to enforce a convention in the order of require statementrequire-order rule to detect require statement with out of order VariableDeclarator
cola119
force-pushed
the
new-require-order-rule
branch
from
cc98cfc to
80c4ae3
Compare
DIY0R
reviewed
Aug 6, 2022
tools/eslint-rules/require-order.js
Outdated
Comment on lines
26
to
28
| node && | ||
| node.id && | ||
| node.id.type === 'ObjectPattern' |
There was a problem hiding this comment.
Suggested change
| node && | |
| node.id && | |
| node.id.type === 'ObjectPattern' | |
| node?.id?.type === 'ObjectPattern' |
cola119
changed the title
require-order rule to detect require statement with out of order VariableDeclaratorrequire-order rule to enforce {a, b, c} = require() is in alphabetical order
cola119
force-pushed
the
new-require-order-rule
branch
from
80c4ae3 to
a4423eb
Compare
Member
LiviaMedeiros
left a comment
LiviaMedeiros
left a comment
There was a problem hiding this comment.
Might be a good idea to include a commit with this rule being applied to the codebase, so it would be easier to estimate the amount of code churn.
Shouldn't something like sort-imports go first?
aduh95
reviewed
Aug 7, 2022
Contributor
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
Could this rule also enforce trailing commas for require/primordials destructuring?
cola119
changed the title
require-order rule to enforce {a, b, c} = require() is in alphabetical ordersort-requires rule to enforce {a, b, c} = require() is in alphabetical order
cola119
force-pushed
the
new-require-order-rule
branch
from
a4423eb to
37d47f0
Compare
cola119
force-pushed
the
new-require-order-rule
branch
from
37d47f0 to
a4c2d41
Compare
aduh95
requested changes
Aug 7, 2022
Contributor
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
We'd need to split this into several commits to minimize the work of backporting. We'd also need to have this rule reject multi-line destructuring without trailing commas.
Member
Author
|
Close on top of #41768 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close #44153
This PR added new custom ESLint rule named
sort-requiresthat verify all destructed members of{} = require()and{} = primordialsare sorted alphabetically inspired byeslint/sort-importsAccording to
make lint-js, there are 221 problems withsort-requires.