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

feat: Add sort-keys rule #76

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

RobertAKARobin
Copy link

@RobertAKARobin RobertAKARobin commented Jan 9, 2025

Prerequisites checklist

What is the purpose of this pull request?

Adds a sort-keys rule for JSON that mimics the functionality of the existing sort-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 this sort-keys maintains parity with that sort-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.

* @fileoverview Tests for sort-keys rule. Cribbed from https://github.com/eslint/eslint/blob/main/tests/lib/rules/sort-keys.js. TODO: How to maintain parity with eslint/sort-keys?

Note that I added a TODO for how comments affect line-separated groups

Copy link

linux-foundation-easycla bot commented Jan 9, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@RobertAKARobin RobertAKARobin force-pushed the feat/sort-keys branch 2 times, most recently from 2c210e9 to d651aae Compare January 9, 2025 14:50
package.json Outdated
@@ -83,6 +83,9 @@
"typescript": "^5.4.5",
"yorkie": "^2.0.0"
},
"peerDependencies": {
"natural-compare": "*"
Copy link
Author

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.

Copy link

@OlivierZal OlivierZal Jan 10, 2025

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).

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to use this package. Let's move in to dependencies instead of peerDependencies.

Copy link
Author

Choose a reason for hiding this comment

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

@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...?

Copy link
Author

Choose a reason for hiding this comment

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

Added more tests for nesting: 3c1270a

Copy link
Member

Choose a reason for hiding this comment

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

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"
   },

Copy link
Author

Choose a reason for hiding this comment

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

Done! a1328df

},

create(context) {
const [directionShort, { caseSensitive, natural, minKeys }] =
Copy link

Choose a reason for hiding this comment

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

allowLineSeparatedGroups not implemented?

Copy link
Author

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!

Copy link
Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Overall this looks good. I left some comments to clean things up.

Also, please add the rule to the rules list in README.md.

package.json Outdated
@@ -83,6 +83,9 @@
"typescript": "^5.4.5",
"yorkie": "^2.0.0"
},
"peerDependencies": {
"natural-compare": "*"
Copy link
Member

Choose a reason for hiding this comment

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

It's fine to use this package. Let's move in to dependencies instead of peerDependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Please add some additional tests here:

  1. When using json/jsonc language, please add some tests that use identifier keys instead of string keys. (For example, { a: true, b: false, _: "foo" }.)
  2. When using json/jsonc language, please add some tests that mix identifier keys and string keys in the same object. (For example, { a: true, "b": false, _: "foo" }.)
  3. Please add some tests with nested objects. (For example, [{ a: true, b: false}, { c: true, d: false }] and { a: { b: true }, c: false }.

Copy link
Author

Choose a reason for hiding this comment

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

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"}.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, are we talking about json/json5? That makes sense, I'll add tests for that.

src/rules/sort-keys.js Outdated Show resolved Hide resolved
@RobertAKARobin
Copy link
Author

Think I addressed all feedback. Thanks!

@mdjermanovic mdjermanovic changed the title Add sort-keys rule. Fixes #75 feat: Add sort-keys rule Jan 12, 2025
Comment on lines 126 to 129
commentLines.has(`${prevLine}:${thisLine}`) ||
commentLines.has(`${prevLine + 1}:${thisLine}`) ||
commentLines.has(`${prevLine}:${thisLine - 1}`) ||
commentLines.has(`${prevLine + 1}:${thisLine - 1}`);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a false negative:

/* eslint json/sort-keys: ["error", "asc", { "allowLineSeparatedGroups": true }] */
{
    "b": 1,
    // some multiline comment
    // using line comment style
    "a": 2 // false negative, "a" and "b" are not line separated
}

Copy link
Member

Choose a reason for hiding this comment

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

Another one:

/* eslint json/sort-keys: ["error", "asc", { "allowLineSeparatedGroups": true }] */
{
    "b": 1
    ,
    "a": 2 // false negative, "a" and "b" are not line separated
}

I think we need to check if there's a blank line between the members (context.sourceCode.lines returns all lines), skipping lines with comments.

Copy link
Author

Choose a reason for hiding this comment

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

Great catch! I'll chew on this and post a fix.

Copy link
Author

@RobertAKARobin RobertAKARobin Jan 14, 2025

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Implementing
Development

Successfully merging this pull request may close these issues.

Feat: sort-keys rule
5 participants