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
Update: Added auto-fix to multiline-ternary #12982
Changes from all commits
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 |
---|---|---|
|
@@ -27,38 +27,23 @@ module.exports = { | |
enum: ["always", "always-multiline", "never"] | ||
} | ||
], | ||
|
||
messages: { | ||
expectedTestCons: "Expected newline between test and consequent of ternary expression.", | ||
expectedConsAlt: "Expected newline between consequent and alternate of ternary expression.", | ||
unexpectedTestCons: "Unexpected newline between test and consequent of ternary expression.", | ||
unexpectedConsAlt: "Unexpected newline between consequent and alternate of ternary expression." | ||
} | ||
}, | ||
|
||
fixable: "whitespace" | ||
}, | ||
|
||
create(context) { | ||
const sourceCode = context.getSourceCode(); | ||
const option = context.options[0]; | ||
const multiline = option !== "never"; | ||
const allowSingleLine = option === "always-multiline"; | ||
|
||
//-------------------------------------------------------------------------- | ||
// Helpers | ||
//-------------------------------------------------------------------------- | ||
|
||
/** | ||
* Tests whether node is preceded by supplied tokens | ||
* @param {ASTNode} node node to check | ||
* @param {ASTNode} parentNode parent of node to report | ||
* @param {boolean} expected whether newline was expected or not | ||
* @returns {void} | ||
* @private | ||
*/ | ||
function reportError(node, parentNode, expected) { | ||
context.report({ | ||
node, | ||
messageId: `${expected ? "expected" : "unexpected"}${node === parentNode.test ? "TestCons" : "ConsAlt"}` | ||
}); | ||
} | ||
|
||
//-------------------------------------------------------------------------- | ||
// Public | ||
//-------------------------------------------------------------------------- | ||
|
@@ -70,23 +55,111 @@ module.exports = { | |
|
||
if (!multiline) { | ||
if (!areTestAndConsequentOnSameLine) { | ||
reportError(node.test, node, false); | ||
context.report({ | ||
node: node.test, | ||
messageId: "unexpectedTestCons", | ||
fix: fixer => { | ||
const testToken = astUtils.isParenthesised(sourceCode, node.test) | ||
? sourceCode.getTokenAfter(node.test) | ||
: node.test; | ||
|
||
const consequentToken = astUtils.isParenthesised(sourceCode, node.consequent) | ||
? sourceCode.getTokenBefore(node.consequent) | ||
: node.consequent; | ||
Comment on lines
+62
to
+68
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. This would produce invalid autofix for a code with multiple parentheses around any of the operands, for example: /* eslint multiline-ternary: ["error", "never"] */
((a)) ?
b : c; will be autofixed to: /* eslint multiline-ternary: ["error", "never"] */
((a) ? b : c; Maybe we could slightly change the logic, to something like how (the same applies to |
||
|
||
return ( | ||
fixer.replaceTextRange( | ||
[ | ||
testToken.range[1], | ||
consequentToken.range[0] | ||
], | ||
" ? " | ||
Comment on lines
+71
to
+76
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. This can remove comments, for example: /* eslint multiline-ternary: ["error", "never"] */
a ? // comment
b : c; will be autofixed to: /* eslint multiline-ternary: ["error", "never"] */
a ? b : c; In my opinion, for this particular rule it would be fine to not autofix code (return (the same applies to |
||
) | ||
); | ||
} | ||
}); | ||
} | ||
|
||
if (!areConsequentAndAlternateOnSameLine) { | ||
reportError(node.consequent, node, false); | ||
context.report({ | ||
node: node.consequent, | ||
messageId: "unexpectedConsAlt", | ||
fix: fixer => { | ||
const consequentToken = astUtils.isParenthesised(sourceCode, node.consequent) | ||
? sourceCode.getTokenAfter(node.consequent) | ||
: node.consequent; | ||
|
||
const alternateToken = astUtils.isParenthesised(sourceCode, node.alternate) | ||
? sourceCode.getTokenBefore(node.alternate) | ||
: node.alternate; | ||
|
||
return ( | ||
fixer.replaceTextRange( | ||
[ | ||
consequentToken.range[1], | ||
alternateToken.range[0] | ||
], | ||
" : " | ||
) | ||
); | ||
} | ||
}); | ||
} | ||
} else { | ||
if (allowSingleLine && node.loc.start.line === node.loc.end.line) { | ||
return; | ||
} | ||
|
||
if (areTestAndConsequentOnSameLine) { | ||
reportError(node.test, node, true); | ||
context.report({ | ||
node: node.test, | ||
messageId: "expectedTestCons", | ||
fix: fixer => { | ||
const testToken = astUtils.isParenthesised(sourceCode, node.test) | ||
? sourceCode.getTokenAfter(node.test) | ||
: node.test; | ||
|
||
const consequentToken = astUtils.isParenthesised(sourceCode, node.consequent) | ||
? sourceCode.getTokenBefore(node.consequent) | ||
: node.consequent; | ||
|
||
return ( | ||
fixer.replaceTextRange( | ||
[ | ||
testToken.range[1], | ||
consequentToken.range[0] | ||
], | ||
"\n? " | ||
) | ||
Comment on lines
+126
to
+133
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. Maybe we could just insert (the same applies to |
||
); | ||
} | ||
}); | ||
} | ||
|
||
if (areConsequentAndAlternateOnSameLine) { | ||
reportError(node.consequent, node, true); | ||
context.report({ | ||
node: node.consequent, | ||
messageId: "expectedConsAlt", | ||
fix: fixer => { | ||
const consequentToken = astUtils.isParenthesised(sourceCode, node.consequent) | ||
? sourceCode.getTokenAfter(node.consequent) | ||
: node.consequent; | ||
|
||
const alternateToken = astUtils.isParenthesised(sourceCode, node.alternate) | ||
? sourceCode.getTokenBefore(node.alternate) | ||
: node.alternate; | ||
|
||
return ( | ||
fixer.replaceTextRange( | ||
[ | ||
consequentToken.range[1], | ||
alternateToken.range[0] | ||
], | ||
"\n: " | ||
) | ||
); | ||
} | ||
}); | ||
} | ||
} | ||
} | ||
|
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.
I'm not sure if this documentation update is necessary.
The same applies to a lot of fixable rules, but we usually don't have these sections since it's reasonable to expect that other formatting rules are also enabled and configured.
operator-linebreak
is already listed in "Related Rules".This detailed explanation with examples is certainly useful, but it's also an additional thing to maintain and keep in sync with other rules.
Thoughts from the team?