New: Allow decimal arguments to indent's VariableDeclarator (fixes #3339) #8975
Conversation
|
|
|
Thanks for the pull request, @sauyon! I took a look to make sure it's ready for merging and found some changes are needed:
Can you please update the pull request to address these? (More information can be found in our pull request guide.) |
|
@sauyon, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @valorkin and @vitorbal to be potential reviewers. |
|
Note: I get ERR! test failed from npm test but the output appears fine: |
|
Thanks for the contribution. I'm not opposed to allowing code like this to be aligned, but I'm also not a big fan of the proposed solution here. Allowing decimal offsets and rounding the result seems like a very hacky workaround to me. If we're going to support alignment, I would prefer an option like |
not-an-aardvark
added
enhancement
|
That does seem better, yeah. I just implemented the suggestion in that issue thread without thinking. I'll play around and see if I can get that set up - though from my cursory glance at the code for the indent rule I suspect it would take some refactoring for it to work properly. |
|
Yeah, I think if we were going to implement that, we would want to refactor the |
|
By the way, you're certainly free to try to implement the |
|
Seems doable, but would require enhancing how WIP implementation (doesn't handle all cases correctly)diff --git a/lib/rules/indent.js b/lib/rules/indent.js
index ec3f7917..d10e66ae 100644
--- a/lib/rules/indent.js
+++ b/lib/rules/indent.js
@@ -508,25 +508,13 @@ module.exports = {
},
VariableDeclarator: {
oneOf: [
- {
- type: "integer",
- minimum: 0
- },
+ ELEMENT_LIST_SCHEMA,
{
type: "object",
properties: {
- var: {
- type: "integer",
- minimum: 0
- },
- let: {
- type: "integer",
- minimum: 0
- },
- const: {
- type: "integer",
- minimum: 0
- }
+ var: ELEMENT_LIST_SCHEMA,
+ let: ELEMENT_LIST_SCHEMA,
+ const: ELEMENT_LIST_SCHEMA
},
additionalProperties: false
}
@@ -631,7 +619,7 @@ module.exports = {
if (context.options[1]) {
lodash.merge(options, context.options[1]);
- if (typeof options.VariableDeclarator === "number") {
+ if (typeof options.VariableDeclarator !== "undefined" && typeof options.VariableDeclarator !== "object") {
options.VariableDeclarator = {
var: options.VariableDeclarator,
let: options.VariableDeclarator,
@@ -759,9 +747,10 @@ module.exports = {
* @param {Token} startToken The start token of the list that element should be aligned against, e.g. '['
* @param {Token} endToken The end token of the list, e.g. ']'
* @param {number|string} offset The amount that the elements should be offset
+ * @param {boolean} force `true` if collapsing should be disabled
* @returns {void}
*/
- function addElementListIndent(elements, startToken, endToken, offset) {
+ function addElementListIndent(elements, startToken, endToken, offset, force) {
/**
* Gets the first token of a given element, including surrounding parentheses.
@@ -781,7 +770,8 @@ module.exports = {
offsets.setDesiredOffsets(
[startToken.range[1], endToken.range[0]],
startToken,
- offset === "first" ? 1 : offset
+ offset === "first" ? 1 : offset,
+ force
);
offsets.setDesiredOffset(endToken, startToken, 0);
@@ -1274,6 +1264,13 @@ module.exports = {
},
VariableDeclaration(node) {
+ const variableKeyword = sourceCode.getFirstToken(node);
+
+ /*
+ * TODO: this is a bit of a hack because addElementsListIndent expects there to be a "last token"
+ * (like a closing array bracket) which is always aligned with the first token.
+ */
+ const semicolon = sourceCode.getLastToken(node);
const variableIndent = options.VariableDeclarator.hasOwnProperty(node.kind) ? options.VariableDeclarator[node.kind] : DEFAULT_VARIABLE_INDENT;
if (node.declarations[node.declarations.length - 1].loc.start.line > node.loc.start.line) {
@@ -1297,16 +1294,13 @@ module.exports = {
* on the same line as the start of the declaration, provided that there are declarators that
* follow this one.
*/
- const firstToken = sourceCode.getFirstToken(node);
-
- offsets.setDesiredOffsets(node.range, firstToken, variableIndent, true);
+ addElementListIndent(node.declarations, variableKeyword, semicolon, variableIndent, true);
} else {
- offsets.setDesiredOffsets(node.range, sourceCode.getFirstToken(node), variableIndent);
+ addElementListIndent(node.declarations, variableKeyword, semicolon, variableIndent);
}
- const lastToken = sourceCode.getLastToken(node);
- if (astUtils.isSemicolonToken(lastToken)) {
- offsets.ignoreToken(lastToken);
+ if (astUtils.isSemicolonToken(semicolon)) {
+ offsets.ignoreToken(semicolon);
}
},
diff --git a/tests/lib/rules/indent.js b/tests/lib/rules/indent.js
index fb621cb4..722d3cee 100644
--- a/tests/lib/rules/indent.js
+++ b/tests/lib/rules/indent.js
@@ -4859,6 +4859,14 @@ ruleTester.run("indent", rule, {
);
}
`
+ },
+ {
+ code: unIndent`
+ const foo = {
+ },
+ baz = 1
+ `,
+ options: [4, { VariableDeclarator: "first" }]
}
],
@@ -9098,6 +9106,20 @@ ruleTester.run("indent", rule, {
</div>
`,
errors: expectedErrors([3, 8, 6, "Block"])
+ },
+ {
+ code: unIndent`
+ const foo = 1,
+ bar = 2,
+ baz = 3
+ `,
+ output: unIndent`
+ const foo = 1,
+ bar = 2,
+ baz = 3
+ `,
+ options: [4, { VariableDeclarator: "first" }],
+ errors: expectedErrors([[2, 6, 4, "Identifier"], [3, 6, 4, "Identifier"]])
}
]
});
|
|
Where does this PR stand? It looks like the corresponding issue was actually closed quite a while ago. |
|
I think #8976 is now the current issue discussing this (although it's discussing a different solution than the one implemented in this PR). |
|
I'm 👎 for this implementation but am 👍 for this issue: #8976 |
|
Closing this PR since there seems to be consensus that it's not the right approach. We appreciate the contribution regardless! Let's move further discussion to #8976. |
eslint-deprecated
bot
added
the
archived due to age
#3339
What is the purpose of this pull request? (put an "X" next to item)
[ ] 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 rule do you want to change?
indent VariableDeclarator
Does this change cause the rule to produce more or fewer warnings?
Neither
How will the change be implemented? (New option, new default behavior, etc.)?
Slightly modified checking and adding a Math.round
Please provide some example code that this change will affect:
What does the rule currently do for this code?
There is no nice way to make eslint accept this code.
What will the rule do after it's changed?
Make eslint accept the code.
What changes did you make? (Give an overview)
Allow number parameters to indent VariableDeclarator, add a Math.round around offset calculation.
Is there anything you'd like reviewers to focus on?