-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add sort-keys rule. Fixes #75 #76
base: main
Are you sure you want to change the base?
Conversation
67968b5
to
7ccba2c
Compare
2c210e9
to
d651aae
Compare
@@ -83,6 +83,9 @@ | |||
"typescript": "^5.4.5", | |||
"yorkie": "^2.0.0" | |||
}, | |||
"peerDependencies": { | |||
"natural-compare": "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate to add a dependency, but since natural-compare
is already a dependency of ESLint then I hope this is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should use natural-compare
to maintain consistency with other sorting rules that rely on it across the ESLint plugins. Just to give you some context to consider: azat-io/eslint-plugin-perfectionist#375 (given that natural-compare
hasn’t been updated in nearly 10 years), but the sorting wouldn't be exactly the same (azat-io/eslint-plugin-perfectionist#384).
src/rules/sort-keys.js
Outdated
}, | ||
|
||
create(context) { | ||
const [directionShort, { caseSensitive, natural, minKeys }] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowLineSeparatedGroups
not implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I added the logic for it, but forgot to add in the option. It's odd that all the tests pass without it. Will look into that today. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Prerequisites checklist
What is the purpose of this pull request?
Adds a
sort-keys
rule for JSON that mimics the functionality of the existingsort-keys
for JS.What changes did you make? (Give an overview)
Added a
sort-keys
rule and tests.Related Issues
Fixes #75
Is there anything you'd like reviewers to focus on?
I started by copying over the existing code for JS
sort-keys
, but it required some big modifications in order to work on JSON. It would be nice if there was an automated way to make sure thissort-keys
maintains parity with thatsort-keys
to keep things consistent across ESLint packages, but JS and JSON are different enough that I'm not sure how it could be done.json/tests/rules/sort-keys.test.js
Line 2 in 7fe2782
Note that I added a TODO for how comments affect line-separated groups