-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 allowLineSeparatedGroups option to sort-keys (fixes #12759) #13573
Changes from 5 commits
d5cdfb4
33e603c
914a498
03d0ac0
f5520c3
ae37ed8
4a2ee2d
6c0e784
365b75d
9393cfb
137b22c
a648a32
1943dc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -61,7 +61,7 @@ let obj = {b: 1, ...c, a: 2}; | |||||
|
||||||
```json | ||||||
{ | ||||||
"sort-keys": ["error", "asc", {"caseSensitive": true, "natural": false, "minKeys": 2}] | ||||||
"sort-keys": ["error", "asc", {"caseSensitive": true, "natural": false, "minKeys": 2, "allowLineSeparatedGroups": false}] | ||||||
} | ||||||
``` | ||||||
|
||||||
|
@@ -75,6 +75,7 @@ The 2nd option is an object which has 3 properties. | |||||
* `caseSensitive` - if `true`, enforce properties to be in case-sensitive order. Default is `true`. | ||||||
* `minKeys` - Specifies the minimum number of keys that an object should have in order for the object's unsorted keys to produce an error. Default is `2`, which means by default all objects with unsorted keys will result in lint errors. | ||||||
* `natural` - if `true`, enforce properties to be in natural order. Default is `false`. Natural Order compares strings containing combination of letters and numbers in the way a human being would sort. It basically sorts numerically, instead of sorting alphabetically. So the number 10 comes after the number 3 in Natural Sorting. | ||||||
* `allowLineSeparatedGroups` - if `true`, Group object keys through line break. Default is `false`. | ||||||
|
||||||
Example for a list: | ||||||
|
||||||
|
@@ -214,6 +215,38 @@ let obj = { | |||||
}; | ||||||
``` | ||||||
|
||||||
### allowLineSeparatedGroups | ||||||
|
||||||
Examples of **incorrect** code for the `{allowLineSeparatedGroups: true}` option: | ||||||
|
||||||
```js | ||||||
/*eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]*// | ||||||
/*eslint-env es6*/ | ||||||
|
||||||
// Each group follows the sort-keys rule. | ||||||
let obj = { | ||||||
c: 2, | ||||||
b: 3, // 'b' should be before 'c'. | ||||||
|
||||||
a: 3 | ||||||
}; | ||||||
``` | ||||||
|
||||||
Examples of **correct** code for the `{allowLineSeparatedGroups: true}` option: | ||||||
|
||||||
```js | ||||||
/*eslint sort-keys: ["error", "asc", {allowLineSeparatedGroups: true}]*// | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
/*eslint-env es6*/ | ||||||
|
||||||
// Divide groups through line breaks. | ||||||
let obj = { | ||||||
b: 2, | ||||||
c: 3, | ||||||
|
||||||
a: 3 | ||||||
}; | ||||||
``` | ||||||
|
||||||
## When Not To Use It | ||||||
|
||||||
If you don't want to notify about properties' order, then it's safe to disable this rule. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,20 @@ function getPropertyName(node) { | |
return node.key.name || null; | ||
} | ||
|
||
/** | ||
* Check the Line Separated Groups | ||
* | ||
* - Compare the start line of the current node with the last line of the previous node. | ||
* If the difference between the two values is greater than 0, Not the same group. | ||
* @param {number} thisStartLine is start line of the current node. | ||
* @param {number} prevEndLine is end line of the previous node. | ||
* @returns {boolean} check group | ||
* @private | ||
*/ | ||
function checkLineSeparatedGroups(thisStartLine, prevEndLine) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we change the function name to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May I change it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
sure :). it makes sense to me |
||
return !!Math.max(thisStartLine - prevEndLine - 1, 0); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has some false positives when we use {
code: `
var obj = {
c: "/*",
a: "*/", // Error. Expected object keys to be in ascending order. 'a' should be before 'c'.
}
`,
options: ["asc", { allowLineSeparatedGroups: true }]
}
🤔 IMO, we can use SourceCode methods for it. there are some methods for treating comment as a getTokenAfter(nodeOrToken, {includeComments: true})
getTokenBefore(nodeOrToken, {includeComments: true}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @yeonjuan !! in this case, var sampleObject1 = {
a: 1,
b: 2,
// comment 1
// comment 2
c: 3
} I used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 (
|
||
|
||
/** | ||
* Functions which check that the given 2 names are in specific order. | ||
* | ||
|
@@ -105,6 +119,10 @@ module.exports = { | |
type: "integer", | ||
minimum: 2, | ||
default: 2 | ||
}, | ||
allowLineSeparatedGroups: { | ||
type: "boolean", | ||
default: false | ||
} | ||
}, | ||
additionalProperties: false | ||
|
@@ -124,6 +142,7 @@ module.exports = { | |
const insensitive = options && options.caseSensitive === false; | ||
const natural = options && options.natural; | ||
const minKeys = options && options.minKeys; | ||
const allowLineSeparatedGroups = options && options.allowLineSeparatedGroups || false; | ||
const isValidOrder = isValidOrders[ | ||
order + (insensitive ? "I" : "") + (natural ? "N" : "") | ||
]; | ||
|
@@ -136,6 +155,7 @@ module.exports = { | |
stack = { | ||
upper: stack, | ||
prevName: null, | ||
prevEndLine: null, | ||
numKeys: node.properties.length | ||
}; | ||
}, | ||
|
@@ -156,13 +176,22 @@ module.exports = { | |
} | ||
|
||
const prevName = stack.prevName; | ||
const prevEndLine = stack.prevEndLine; | ||
const numKeys = stack.numKeys; | ||
const thisName = getPropertyName(node); | ||
const thisStartLine = node.loc.start.line; | ||
const isLineSeparatedGroups = prevEndLine && checkLineSeparatedGroups(thisStartLine, prevEndLine); | ||
|
||
stack.prevEndLine = node.loc.end.line; | ||
|
||
if (thisName !== null) { | ||
stack.prevName = thisName; | ||
} | ||
|
||
if (allowLineSeparatedGroups && isLineSeparatedGroups) { | ||
return; | ||
} | ||
|
||
if (prevName === null || thisName === null || numKeys < minKeys) { | ||
return; | ||
} | ||
|
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.