-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Conversation
Thanks for contributing. Just so you're aware, since there isn't an accepted issue for this, we'll be starting the consensus-seeking process outlined here. This can take some time, so we ask for your patience. It's fine to wait to implement them until the team has decided whether or not we will accept this, but we also require corresponding tests for any changes in the codebase. |
@kaicataldo Of course! I'll make sure to add tests as well :). |
I support this enhancement 👍 Wondering was there a specific reason why this rule isn't already fixable, almost all other whitespace rules are. |
@kaicataldo The reason I made this PR is actually because I had an issue with ternary auto-formatting. Without auto-fixing So even though 'operator-linebreak': [
'warn',
'before',
], |
Can you please provide an example where this happens? |
Came across this same code with different variable names in a project at work. This isn't the only instance, but it's a recurring example. items = isSomething ? [
...stuff,
...moreStuff,
] : collection.items Nothing happens even though the items = isSomething
? [
...stuff,
...moreStuff,
]
: collection.items ^^ It's indenting the ternary like that because of The only rule change between these is
|
It's now passing all unit tests with The output results are a little wonky, but they work correctly if you use it together with I updated the docs to reflect this factor. |
I noticed unfixed warnings from this ESLint rule and wanted to auto-fix them. While it may seem like doing `'\n? '` is opinionated, I did have changed to make this take a new option and either put the `?` on the previous line or next line. This is actually unnecessary because `operator-linebreak` handles it for you.
👍 I agreed with this enhancement. |
It looks like we just need a champion to accept this. |
it is accepted now! 🎉 |
### --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. |
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?
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; |
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.
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)
)
fixer.replaceTextRange( | ||
[ | ||
testToken.range[1], | ||
consequentToken.range[0] | ||
], | ||
" ? " |
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.
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)
).
return ( | ||
fixer.replaceTextRange( | ||
[ | ||
testToken.range[1], | ||
consequentToken.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.
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)
)
#13195 might be relevant to this. |
@Sawtaytoes Anything else we can do to help you land this? |
Hey, I can take a look again this weekend and see what needs done. |
Sadly, I didn't have time last weekend. |
It looks like #13367 has been merged. @Sawtaytoes can you rebase onto master? |
@Sawtaytoes it would be great to have this feature, is there anything we can do to help? |
@mdjermanovic If you wanna pair code, that'd be great. At this point, it's been so long I'd need to refresh myself on what I was even doing, how I tested, etc. I had a second kid since I made the original PR, so I haven't had as much time to spend in off-work coding. But if I had someone to pair with, I'm sure we could figure it out, write up any remaining requirements for someone else to pick up (or me), and get this merged! |
What else needs to be done to get this merged in? |
@vegerot it looks like we would need someone to take over this PR to get it moving again. |
This should be easier with the current version of this rule. All relevant tokens are already there, and there's no need to check for parentheses. eslint/lib/rules/multiline-ternary.js Lines 50 to 57 in 145aec1
It's fine to just return Documentation update doesn't seem necessary. |
ha, thanks @mdjermanovic pointing! I can take over and update it later. |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[X] 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)
Added functionality to automatically fix
multiline-ternary
issues.Is there anything you'd like reviewers to focus on?
While it may seem like doing
'\n? '
is opinionated, I did have changed to make this take a new option and either put the?
on the previous line or next line. This is actually unnecessary becauseoperator-linebreak
handles it for you.This will most-likely have to be reflected in the docs as well similar to how
indent
is required for every rule's ability to properly fix tabbing.