tools: add sort-requires rule to enforce {a, b, c} = require() is in alphabetical order - #44116
Closed
cola119 wants to merge 2 commits into
Closed
tools: add sort-requires rule to enforce {a, b, c} = require() is in alphabetical order#44116cola119 wants to merge 2 commits into
cola119 wants to merge 2 commits into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
cola119
force-pushed
the
new-require-order-rule
branch
from
cc98cfc to
80c4ae3
Compare
August 3, 2022 15:36
DIY0R
reviewed
Aug 6, 2022
Comment thread
tools/eslint-rules/require-order.js
Outdated
Comment on lines
26
to
28
Copy link
Copy Markdown
There was a problem hiding this comment.
Suggested change
| node && | |
| node.id && | |
| node.id.type === 'ObjectPattern' | |
| node?.id?.type === 'ObjectPattern' |
Sorry, something went wrong.
cola119
force-pushed
the
new-require-order-rule
branch
from
80c4ae3 to
a4423eb
Compare
August 6, 2022 13:43
LiviaMedeiros
left a comment
Copy link
Copy Markdown
Member
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?
Sorry, something went wrong.
aduh95
reviewed
Aug 7, 2022
aduh95
left a comment
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Could this rule also enforce trailing commas for require/primordials destructuring?
Sorry, something went wrong.
Comment thread
tools/eslint-rules/require-order.js
Outdated
Comment thread
tools/eslint-rules/require-order.js
Outdated
cola119
force-pushed
the
new-require-order-rule
branch
from
a4423eb to
37d47f0
Compare
August 7, 2022 15:29
cola119
force-pushed
the
new-require-order-rule
branch
from
37d47f0 to
a4c2d41
Compare
August 7, 2022 15:33
cola119
marked this pull request as ready for review
August 7, 2022 15:34
aduh95
requested changes
Aug 7, 2022
aduh95
left a comment
Copy link
Copy Markdown
Contributor
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.
Sorry, something went wrong.
cola119
marked this pull request as draft
August 8, 2022 03:29
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
Reviewers
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-requires that verify all destructed members of {} = require() and {} = primordials are sorted alphabetically inspired by eslint/sort-imports
According to make lint-js, there are 221 problems with sort-requires.