feat: add allowLineSeparatedGroups option to sort-keys (fixes #12759) #13573

JJoGeon · 2020-08-16T14:42:02Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Add allowLineSeparatedGroups option to sort-keys rule #12759

Is there anything you'd like reviewers to focus on?

None

yeonjuan · 2020-08-17T07:48:40Z

lib/rules/sort-keys.js

+                const isLineSeparatedGroups = prevEndLine && checkLineSeparatedGroups(thisStartLine, prevEndLine);
+
+                if (thisStartLine !== null) {
+                    stack.prevEndLine = thisStartLine;


It seems to treat the multiline property as a group separator because it compares the start line of the previous property and the start line of the current property. So the below case has no lint error.

{ code: ` var obj = { b: 1, // 1st Group c () { // 1st Group }, a: 3 // 2nd Group ?? No Error } `, options: ["asc", { allowLineSeparatedGroups: true }], parserOptions: { ecmaVersion: 6 } },

Maybe, we should compare the end line of the previous property with the start line of the current property as what "sort-import" does

demo

/*eslint sort-imports: ["error", { "allowSeparatedGroups": true }]*/ import b from 'foo.js'; import d from 'bar.js'; import a from 'baz.js'; // Lint error, cause it's not separated from previous

Sorry !!!!
I made a mistake while clean the code.

It was stack.prevEndLine = node.loc.end.line; before PR.

Thank you !

Sorry !!!!
I made a mistake while clean the code.

There's no need to be sorry :)
Can we add a test case for ensuring it? (maybe in invalid cases?)

{ code: ` var obj = { b: 1, c () { }, a: 3 } `, options: ["asc", { allowLineSeparatedGroups: true }], parserOptions: { ecmaVersion: 6 } },

yeonjuan

Thanks for the contribution 👍 I left some comments :)

yeonjuan · 2020-08-17T08:09:23Z

lib/rules/sort-keys.js

                    }
                },
                additionalProperties: false
            }
        ],

        messages: {
+            allowLineSeparatedGroups: "'{{thisName}}' should be before '{{prevName}}'. To separate groups by line break, apply the allowLineSeparatedGroups option.",


I'm not sure it's good to give option information to those who don't use this option.
I want to hear other member's opinion

I think we should remove this message, and just use the existing one. I believe we don't have any error messages that advise users to reconfigure the rule.

Thanks for the advice !!
I'll remove it !!

yeonjuan

@JJoGeon Thanks for the change :) I left some comments.

from outdated comment

I made a mistake while clean the code.
It was stack.prevEndLine = node.loc.end.line; before PR.

Can we add a test case for ensuring it? (maybe in invalid cases?)

{
   code: `
     var obj = {
        b: 1, 
        c () { 

        },
        a: 3
      }
     `,
    options: ["asc", { allowLineSeparatedGroups: true }],
    parserOptions: { ecmaVersion: 6 },
    errors : [/* ... */]
},

yeonjuan · 2020-08-17T17:00:44Z

lib/rules/sort-keys.js

+ * @returns {number} number of lines between nodes.
+ * @private
+ */
+function checkLineSeparatedGroups(thisStartLine, prevEndLine) {


Can we change the function name to getLinesBetween or getNumberOfLinesBetween?
IMO check prefixed name seems to represent return boolean or call context.report() but actually it returns the number of lines between two.

May I change it to !!Math.max(thisStartLine - prevEndLine - 1, 0); ?
In this function, distances the value of the two is not important but whether they are separated from each other is important.

May I change it to !!Math.max(thisStartLine - prevEndLine - 1, 0); ?

sure :). it makes sense to me

yeonjuan · 2020-08-17T17:12:19Z

lib/rules/sort-keys.js

+                const thisStartLine = node.loc.start.line;
+                const isLineSeparatedGroups = prevEndLine && checkLineSeparatedGroups(thisStartLine, prevEndLine);
+
+                if (thisStartLine !== null) {


const thisStartLine = node.loc.start.line; //... if (thisStartLine !== null) {

I think it's unneeded condition since thisStartLine seems equals with node.loc.start.line (number).
so thisStartLine !== null seems always true

JJoGeon · 2020-08-18T13:27:39Z

Modification is complete!
Please check again !

thank you !

mdjermanovic · 2020-08-18T21:19:06Z

Should the option insist on a blank line, as in the original issue, or just check if the properties aren't on consecutive lines as it is implemented in the actual iteration.

For example:

/* eslint sort-keys: ["error", "asc", { "allowLineSeparatedGroups": true} ] */

var obj = {
    b,
    // comment
    a
}

mdjermanovic

Also, the actual rule skips computed keys (except for simple ones), but compares non-computed keys around them:

/* eslint sort-keys: ["error", "asc"] */

var obj = {
    b,
    [foo + bar]: 1,
    a // sort-keys: 'a' should be before 'b'
}

online demo

If the option is "allowLineSeparatedGroups": true, what should be checked in the above example?

In the actual iteration, this code would produce an error, while it probably shouldn't:

/* eslint sort-keys: ["error", "asc", { "allowLineSeparatedGroups": true } ] */

var obj = {
  b,
  
  [foo + bar]: 1,
  a // sort-keys: 'a' should be before 'b'
}

yeonjuan · 2020-08-22T06:09:08Z

docs/rules/sort-keys.md

+Examples of **incorrect** code for the `{allowLineSeparatedGroups: true}` option:
+
+```js
+/*eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]*//


Suggested change

/*eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]*//

/*eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]*/

yeonjuan · 2020-08-22T06:09:18Z

docs/rules/sort-keys.md

+Examples of **correct** code for the `{allowLineSeparatedGroups: true}` option:
+
+```js
+/*eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]*//


Suggested change

/*eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]*//

/*eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]*/

mdjermanovic · 2020-08-22T11:55:58Z

We recently added allowSeparatedGroups to the sort-imports rule, and that option indeed checks if the imports are on consecutive lines.

However, for sort-keys it might make more sense to check for a blank line, in order to avoid problems and ambiguous behavior in cases like this one.

@yeonjuan, @JJoGeon what do you think of this?

yeonjuan · 2020-08-22T13:15:46Z

HI @mdjermanovic :)

However, for sort-keys it might make more sense to check for a blank line, in order to avoid problems and ambiguous behavior in cases like this one.

for double-check, Is this mean a comment line cannot create groups?

/* eslint sort-keys: ["error", "asc", { "allowLineSeparatedGroups": true} ] */

var obj = {
    b,
    [foo + bar]: 1,
    a  // lint-error
}

var obj = {
    b,
    [foo + bar]: 1,
    // comments
    a  // lint-error
}

var obj = {
    b,
    [foo + bar]: 1,
    // comments

    a  // lint-free
}

var obj = {
    b,
    [foo + bar]: 1,

    // comments
    a  // lint-free
}

mdjermanovic · 2020-08-22T22:08:29Z

for double-check, Is this mean a comment line cannot create groups?

Yes, I think comment lines shouldn't create groups. That way, I believe it would be easier to explain this option in the docs, and much easier for users to understand how this option exactly works: it simply checks if there are any blank lines between properties. That's just my opinion, it could be wrong.

I also tried the tslint rule mentioned in the original post of the issue, and it seems to be working like that: TSLint playground link

(we don't have to match the behavior, but it looks reasonable to me)

JJoGeon · 2020-08-23T05:50:57Z

HI @mdjermanovic @yeonjuan !!

Thank you for the good Opinion!
I think comment lines shouldn't create groups, as mdjermanovic said.

var sampleObject1 = {
  a: 1,
  b: 2,
  // c description
  c: 3
} // one object, one group

var sampleObject2 = {
  a: 1,
  b: 2,
  // another group description
  c: 3
} // one object, two group

We don't know what that comment means. (eslint too.)
So I think it's better to judge by a different group only when there is a separated line.

var sampleObject1 = {
  a: 1,
  b: 2,
  // c or another group description
  c: 3
} // one object, one group

var sampleObject2 = {
  a: 1,
  b: 2,

  // c or another group description
  c: 3
} // one object, two group

yeonjuan · 2020-08-23T07:30:06Z

@mdjermanovic

Yes, I think comment lines shouldn't create groups. That way, I believe it would be easier to explain this option in the docs, and much easier for users to understand how this option exactly works:

I agree with it 👍 thanks for the reply

JJoGeon · 2020-08-29T08:31:56Z

Hi @yeonjuan, @mdjermanovic ! :)

Finish comment line check logic.

JJoGeon · 2020-08-29T08:38:13Z

lib/rules/sort-keys.js

+ */
+function checkLineSeparatedGroups(thisStartLine, prevEndLine, sourceCodeLines) {
+    let blockCommentFlag = false;
+    let commentOffset = 0;


@yeonjuan @mdjermanovic

I used the forEach function to check the comment line, but I'm not sure if it's okay.

Is there a better way?

yeonjuan

@JJoGeon
Thanks for keep working it.
I left comments about false positives and suggestions.

yeonjuan · 2020-09-01T12:34:41Z

lib/rules/sort-keys.js

+    });
+
+    return !!Math.max(thisStartLine - prevEndLine - commentOffset - 1, 0);
+}


It has some false positives when we use /*, */, // as a value in code.

{ code: ` var obj = { c: "/*", a: "*/", // Error. Expected object keys to be in ascending order. 'a' should be before 'c'. } `, options: ["asc", { allowLineSeparatedGroups: true }] }

Is there a better way?

🤔 IMO, we can use SourceCode methods for it. there are some methods for treating comment as a node or token

getTokenAfter(nodeOrToken, {includeComments: true}) getTokenBefore(nodeOrToken, {includeComments: true});

Thank you @yeonjuan !!

in this case,

var sampleObject1 = { a: 1, b: 2, // comment 1 // comment 2 c: 3 }

I used getTokensBefore.

I think this would be a complicated way to find out if there's a blank line.

For example, this is a false positive:

/* eslint sort-keys: ["error", "asc", { allowLineSeparatedGroups: true }] */ var obj = { b, /* */ // a }

comments offset is calculated as 3, but it doesn't take into account if there are lines between comments or not.

Could we maybe simplify this? We could take tokens between properties and then see if there's a blank line between any two successive tokens.

More precisely, if we want to check if there's a blank line between two properties (property1 and property2), than we could check if there's a blank line between any two of the following items:

property1

comma after property1

comment1

comment2

....

commentN

property2

JJoGeon · 2020-09-07T15:40:54Z

Hi, @mdjermanovic !! :)

I modified the code to pass all test cases.
review and check please.

thank you !

mdjermanovic · 2020-09-14T01:10:28Z

Hi, @mdjermanovic !! :)

I modified the code to pass all test cases.
review and check please.

thank you !

@JJoGeon thanks for the changes and sorry for the delay from my side! I left a comment here

JJoGeon · 2020-10-10T12:32:21Z

Hi, @mdjermanovic !! :)
sorry for the delay from my side.

I improved the getCommentOffset function !!

Please check the upgraded getCommentOffset function!

Thank you for your continued good opinion.

mdjermanovic · 2020-11-02T17:46:16Z

This wouldn't work well with the comma-first style.

For example, this would be a false negative:

/* eslint sort-keys: ["error", "asc", { "allowLineSeparatedGroups": true }] */

var obj = {
    b: 1
  // comment
  , a: 2
};

Can we maybe check if there's a blank line between two properties by checking for blank lines between consecutive nodes/tokens (incl. comments), instead of calculating comments offset?

[property1, comment, comment, ... , comma, comment, comment, ... , property2]

nzakas · 2021-03-25T00:47:46Z

@JJoGeon are you still working on this?

JJoGeon · 2021-03-25T01:58:31Z

@nzakas yes.
Thank you remind me.

For personal reasons, i couldn't work this issue.
My personal problem is solved well now, so I'm going to proceed again soon.

nzakas · 2021-03-25T19:56:54Z

@JJoGeon great, happy to hear it!

JJoGeon · 2021-05-06T13:40:39Z

@mdjermanovic
Thank you for your opinion and feedback.

I solved the comma first style problem you told me.

Can we maybe check if there's a blank line between two properties by checking for blank lines between consecutive nodes/tokens (incl. comments), instead of calculating comments offset?

But this suggestion not solved.
I looked up functions for context.getSourceCode() on this site. (Link)
but no function was found to determine if there was a line break between the two nodes.

Then isn't there only a way to calculate the offset with the line information of the two nodes?

I want to finish with a better code.
Can you give me additional ideas or opinions?

nzakas · 2021-05-25T00:13:19Z

@mdjermanovic please take a look

mdjermanovic · 2022-01-27T13:24:41Z

@JJoGeon sorry for the delay. I thought I left an answer, but apparently I didn't.

Can we maybe check if there's a blank line between two properties by checking for blank lines between consecutive nodes/tokens (incl. comments), instead of calculating comments offset?

But this suggestion not solved. I looked up functions for context.getSourceCode() on this site. (Link) but no function was found to determine if there was a line break between the two nodes.

There isn't a built-in helper function to determine if there is a blank line between two nodes, but we can easily determine that by checking their lines: If a and b are two consecutive nodes/tokens/comments, then b.loc.start.line - a.loc.end.line > 1 means that there's a blank line between them.

JJoGeon · 2022-02-21T03:44:37Z

@mdjermanovic Thank you for your advice.

I'll wrap it up soon and PR again.
Thank you so much for your care.

alexthehurst · 2022-05-13T19:30:30Z

@JJoGeon Really looking forward to this improvement if you or anyone is able to push it forward :)

mvuherer · 2022-06-28T15:29:54Z

@JJoGeon any updates on this?

snitin315 · 2022-06-28T15:56:05Z

I'll finish this.

mvuherer · 2022-06-28T16:02:22Z

I'll finish this.

Thank you very much! You are the hero we don't deserve.

JJoGeon · 2022-06-29T08:42:24Z

Thank you for your interest in my PR.
For personal reasons, I didn't care.

I'll try to finish it as soon as possible.

But it's okay for good people to finish before I finish. (@alexthehurst , @mvuherer , @snitin315)
Thank you and sorry.

snitin315 · 2022-07-17T03:07:48Z

Closing in favor of #16138

JJoGeon added 2 commits August 16, 2020 21:33

sort-keys, add allowLineSeparatedGroups option

d5cdfb4

update sort-keys document.

33e603c

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Aug 16, 2020

JJoGeon changed the title ~~Issue 12759~~ Add allowLineSeparatedGroups option to sort-keys rule #12759 Aug 16, 2020

anikethsaha changed the title ~~Add allowLineSeparatedGroups option to sort-keys rule #12759~~ Update: add allowLineSeparatedGroups option to sort-keys (fixes #12759) Aug 16, 2020

yeonjuan added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 17, 2020

yeonjuan reviewed Aug 17, 2020

View reviewed changes

yeonjuan suggested changes Aug 17, 2020

View reviewed changes

prevEndLine Value Change

914a498

yeonjuan suggested changes Aug 17, 2020

View reviewed changes

JJoGeon added 2 commits August 18, 2020 20:56

remove message and update checkLineSeparatedGroups function.

03d0ac0

add new invalid case

f5520c3

mdjermanovic requested changes Aug 18, 2020

View reviewed changes

yeonjuan reviewed Aug 22, 2020

View reviewed changes

mdjermanovic linked an issue Aug 22, 2020 that may be closed by this pull request

Add allowLineSeparatedGroups option to sort-keys rule #12759

Closed

JJoGeon added 2 commits August 23, 2020 15:04

if line separated group, init before stack

ae37ed8

Remove Unnecessary Characters.

4a2ee2d

JJoGeon added 2 commits August 29, 2020 17:29

Add comment line check logic.

6c0e784

Add comment line test case.

365b75d

JJoGeon commented Aug 29, 2020

View reviewed changes

yeonjuan suggested changes Sep 1, 2020

View reviewed changes

improve getCommentOffset logic and change another group check logic.

9393cfb

update getCommentOffset function docs & remove Object.values

137b22c

getCommentOffset function improvement.

a648a32

Resolving the comma-first style issue.

1943dc8

mdjermanovic changed the title ~~Update: add allowLineSeparatedGroups option to sort-keys (fixes #12759)~~ feat: add allowLineSeparatedGroups option to sort-keys (fixes #12759) Oct 26, 2021

snitin315 closed this Jul 17, 2022

eslint-github-bot bot locked and limited conversation to collaborators Jan 14, 2023

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 14, 2023

	/eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]//
	/eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]/

feat: add allowLineSeparatedGroups option to sort-keys (fixes #12759) #13573

feat: add allowLineSeparatedGroups option to sort-keys (fixes #12759) #13573

Conversation

JJoGeon commented Aug 16, 2020 • edited

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

yeonjuan Aug 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeonjuan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeonjuan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeonjuan Aug 17, 2020 • edited

Choose a reason for hiding this comment

JJoGeon commented Aug 18, 2020

mdjermanovic commented Aug 18, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjermanovic commented Aug 22, 2020

yeonjuan commented Aug 22, 2020 • edited

mdjermanovic commented Aug 22, 2020

JJoGeon commented Aug 23, 2020

yeonjuan commented Aug 23, 2020

JJoGeon commented Aug 29, 2020

Choose a reason for hiding this comment

yeonjuan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JJoGeon Sep 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JJoGeon commented Sep 7, 2020

mdjermanovic commented Sep 14, 2020

JJoGeon commented Oct 10, 2020

mdjermanovic commented Nov 2, 2020

nzakas commented Mar 25, 2021

JJoGeon commented Mar 25, 2021

nzakas commented Mar 25, 2021

JJoGeon commented May 6, 2021 • edited

nzakas commented May 25, 2021

mdjermanovic commented Jan 27, 2022

JJoGeon commented Feb 21, 2022

alexthehurst commented May 13, 2022

mvuherer commented Jun 28, 2022

snitin315 commented Jun 28, 2022

mvuherer commented Jun 28, 2022

JJoGeon commented Jun 29, 2022

snitin315 commented Jul 17, 2022

JJoGeon commented Aug 16, 2020 •

edited

yeonjuan Aug 17, 2020 •

edited

yeonjuan left a comment •

edited

yeonjuan left a comment •

edited

yeonjuan Aug 17, 2020 •

edited

yeonjuan commented Aug 22, 2020 •

edited

yeonjuan left a comment •

edited

JJoGeon Sep 7, 2020 •

edited

JJoGeon commented May 6, 2021 •

edited