Add sort-keys rule. Fixes #75 #76
Conversation
RobertAKARobin
force-pushed
the
feat/sort-keys
branch
from
67968b5 to
7ccba2c
Compare
RobertAKARobin
force-pushed
the
feat/sort-keys
branch
from
2c210e9 to
d651aae
Compare
package.json
Outdated
| @@ -83,6 +83,9 @@ | |||
| "typescript": "^5.4.5", | |||
| "yorkie": "^2.0.0" | |||
| }, | |||
| "peerDependencies": { | |||
| "natural-compare": "*" | |||
There was a problem hiding this comment.
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.
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).
There was a problem hiding this comment.
It's fine to use this package. Let's move in to dependencies instead of peerDependencies.
There was a problem hiding this comment.
@nzakas Should it be pegged to a specific version? I used peerDependencies and no specific version since natural-compare is a dependency of eslint here, and eslint will presumably always be installed alongside eslint/json, so presumably we would want to defer to whatever version of natural-compare is required by eslint?
Although I notice eslint is a devDependency of eslint/json and not a dependency here, so maybe that's not correct...?
There was a problem hiding this comment.
Added more tests for nesting: 3c1270a
There was a problem hiding this comment.
I also think this should be added to dependencies.
"dependencies": {
"@eslint/plugin-kit": "^0.2.3",
"@humanwhocodes/momoa": "^3.3.4",
+ "natural-compare": "^1.4.0"
},
src/rules/sort-keys.js
Outdated
| }, | ||
|
|
||
| create(context) { | ||
| const [directionShort, { caseSensitive, natural, minKeys }] = |
There was a problem hiding this comment.
allowLineSeparatedGroups not implemented?
There was a problem hiding this comment.
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!
package.json
Outdated
| @@ -83,6 +83,9 @@ | |||
| "typescript": "^5.4.5", | |||
| "yorkie": "^2.0.0" | |||
| }, | |||
| "peerDependencies": { | |||
| "natural-compare": "*" | |||
There was a problem hiding this comment.
It's fine to use this package. Let's move in to dependencies instead of peerDependencies.
tests/rules/sort-keys.test.js
Outdated
There was a problem hiding this comment.
Please add some additional tests here:
- When using
json/jsonclanguage, please add some tests that use identifier keys instead of string keys. (For example,{ a: true, b: false, _: "foo" }.) - When using
json/jsonclanguage, please add some tests that mix identifier keys and string keys in the same object. (For example,{ a: true, "b": false, _: "foo" }.) - Please add some tests with nested objects. (For example,
[{ a: true, b: false}, { c: true, d: false }]and{ a: { b: true }, c: false }.
There was a problem hiding this comment.
As written, eslint/json still requires JSONC files to have string keys I think? When specifying language: "json/jsonc" the parser fails with Parsing error: Unexpected character on {foo: "bar"} but correctly parses {"foo": "bar"}.
There was a problem hiding this comment.
Oh, are we talking about json/json5? That makes sense, I'll add tests for that.
Co-authored-by: Nicholas C. Zakas <[email protected]>
|
Think I addressed all feedback. Thanks! |
RobertAKARobin
force-pushed
the
feat/sort-keys
branch
from
7d9916f to
69211e8
Compare
Prerequisites checklist
What is the purpose of this pull request?
Adds a
sort-keysrule for JSON that mimics the functionality of the existingsort-keysfor JS.What changes did you make? (Give an overview)
Added a
sort-keysrule 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-keysmaintains parity with thatsort-keysto 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