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.

Sign up for GitHub

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

Jump to bottom

Update: Added auto-fix to multiline-ternary #12982

Closed
wants to merge 1 commit into from
Closed

Update: Added auto-fix to multiline-ternary #12982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 56 additions & 0 deletions docs/rules/multiline-ternary.md
View file
Open in desktop
Expand Up @@ -136,6 +136,62 @@ foo > bar ? value1 : value2;
foo > bar ? (baz > qux ? value1 : value2) : value3;
```

### --fix

If this rule is invoked with the command-line `--fix` option, it's recommended to define both `indent` and `operator-linebreak` if you want to have sensible results when using the `always` and `always-multiline` options.
Comment on lines +139 to +141
Copy link
Member

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?


For instance, this code:

```js
const func = () => {
const items = hasStuff ? [
...stuff.items,
...previousStuff.items,
] : previousStuff.items
}
```

Is converted to:

```js
const func = () => {
const items = hasStuff
? [
...stuff.items,
...previousStuff.items,
]
: previousStuff.items
}
```

But can be converted to depending on your `indent` value:

```js
const func = () => {
const items = hasStuff
? [
...stuff.items,
...previousStuff.items,
]
: previousStuff.items
}
```

Or even this way depending on your `operator-linebreak` value:

```js
const func = () => {
const items = hasStuff ?
[
...stuff.items,
...previousStuff.items,
] :
previousStuff.items
}
```

The way it choses how to automatically fix depends on how your ternaries were formatted prior to running `--fix`, but with `indent` and `operator-linebreak`, you'll achieve consistent results.

## When Not To Use It

You can safely disable this rule if you do not have any strict conventions about whether the operands of a ternary expression should be separated by newlines.
Expand Down
121 changes: 97 additions & 24 deletions lib/rules/multiline-ternary.js
View file
Open in desktop
Expand Up @@ -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
//--------------------------------------------------------------------------
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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 operator-linebreak finds these tokens.

(the same applies to if (!areConsequentAndAlternateOnSameLine))


return (
fixer.replaceTextRange(
[
testToken.range[1],
consequentToken.range[0]
],
" ? "
Comment on lines +71 to +76
Copy link
Member

Choose a reason for hiding this comment

The 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 null) if there are any comments within the range between the two tokens.

(the same applies to if (!areConsequentAndAlternateOnSameLine)).

)
);
}
});
}

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
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could just insert \n before ? instead of replacing the range? I believe all other newline rules work that way.

(the same applies to if (areConsequentAndAlternateOnSameLine))

);
}
});
}

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: "
)
);
}
});
}
}
}
Expand Down