prettier
prettier copied to clipboard
prettier
Nested ternary formatting - add indents back
Edit: I will no longer be actively participating in this thread - my reasons are explained in comment below.
Hello,
there are many - including me thinking that change to nested ternary is not best decision.
Eg threads https://github.com/prettier/prettier/issues/5476 https://github.com/prettier/prettier/issues/3018
I know there is some reasoning why nested ternary indent was removed, but it's much harder to read code now.
At least would be good to have option to remain nice formatting (from previous prettier version) - formatting of ternaries with indent and not under each other like:
condition
? a
: b
? c
: d
instead much easier to understand and read is:
condition
? a
: b
? c
: d
Hi!
there are many - including me thinking that change to nested ternary is not best decision.
Eg threads #5476 #3018
As far as I can tell #3018 has nothing to do with this, but #5476 sure does. That leaves us with … 3 people as of this writing who has expressed wanting the old behavior.
EDIT: There were only 3 upvotes when this was written. Now there's more than 30 times that.
@lydell to be honestly indent in nested ternary was very very very comfortable, old behavior was better and readable, it is really impossible to read nested ternary (especial with nested 3)
Example https://github.com/prettier/prettier/issues/5681#issuecomment-449952135, also we can close this in favor #5681 (i think)
@jurosh
there are many - including me thinking that change to nested ternary is not best decision
There were many that thought the previous formatting wasn't good.
At least would be good to have option
That's unlikely to happen
@evilebottnawi
The first attempt to change this was made in Apr 2017, so this was an old issue that we wanted to fix. This change was long desired.
I personally have no preference, but we can't keep changing things over and over.
I personally prefer the new style, but I also don't think that matters.
The PR at hand, for reference, is https://github.com/prettier/prettier/pull/5039
I think we should leave this issue open for a while to see how many upvotes it gets. If it isn't a lot by the time we hit prettier 2.0, I would suggest we close - better to avoid churn.
Not sure why this happens and if it's related to this issue, but the following prettier formatting literally kills me.. any way to somehow make it just like the source?
Personally I prefer neither. Unless the branches are themselves are big, I prefer something like:
a ? ifA :
b ? ifB :
c ? (
d ? ifD : elseD
) : neither
but I am... ok with the current formatting. The old formatting would cause deep trees even when there was no actual nesting, only chaining.
FWIW, I only consider ternaries "nested" when there is an additional ternary in the ? branch, not in the : branch. Even if it technically is nested in the AST, I find it easier to think of it as chaining than nesting.
I'm actually considering something similar to what we do for JSX, though I'm not sure about it:
const result = type === 'first' ? (
getFirstThing()
) : type === 'second' ? (
getSecondThing()
) : (
getOtherThing()
)
Thoughts?
EDIT:
I find it easier to think of it as chaining than nesting.
Agreed. I think continuing nest additional ternaries in the ? branch is a good idea.
is there a way to make it configurable? like a template or something? maybe someone could guide me so I could create a PR.
That would really solve our long-going debates within my team..
I also prefer the old way (with indentation).
I feel like readability is hugely impacted with the new way.
But I agree with @ilyadoroshin - this should probably be configurable (rather than enforced).
@rattrayalex , your suggestion is good, the format is handled correctly by the ESLint "indent" rule and by VS Code auto-indent.
For pure JS code the easiest way to solve issues with nested ternaries is to stop using them in favor of if+else. It's not that easy with TypeScript, though, as the type language doesn't have if but it does have a ternary type. Consider this type helper to infer React component props from a component:
type ReactPropsType<T> = T extends Component<infer P1, any>
? P1
: T extends FunctionComponent<infer P2>
? P2
: never;
This doesn't read all that well; the following formatting:
type ReactPropsType<T> = T extends Component<infer P1, any>
? P1
: T extends FunctionComponent<infer P2>
? P2
: never;
would make the logic clearer IMO.
That said, I can live with the current behavior if more people prefer it. And I agree limiting churn is also valuable.
@mgol and all those who want indented ternaries, please try eslint-plugin-prettierx, based on Prietterx that supports these and other options that make it more compatible with ESLint/StandardJS.
The plugin is WIP but usable and with your help it will improve.
On the road I found some issues and I will try to send a RP to this repo to solve the most importants (related with ASI, my preferred style).
PD: Indented ternaries (ESLint-like) is the default with the "standardx" and "standardize" presets of the plugin.
I do hesitate to add "me too" comments, but it looks like that's why this issue is open and formatting mostly opinion after all.
The original (nested) formatting of ternaries was one of the biggest draws of prettier for me, and when it changed, I just assumed it was a bug and would be restored. We write a lot of functional-style code that is heavy on ternaries, and our codebase has gone from pristine to nearly unreadable with this change.
Even in simple cases, this change is deadly for readability:
I know that prettier is opinionated and shies away from configuration, but seeing as this is changed behavior I really do expect an option to preserve the original style.
After some effort on my part, prettier is heavily adopted within my organization, and it would be difficult for me to push everyone to something else like prettierx, especially when there is so little certainty around its continued maintenance, compared with prettier.
I am happy to submit a PR this week to make this behavior configurable. If I do and it passes quality checks, will the team merge it?
I am happy to submit a PR this week to make this behavior configurable. If I do and it passes quality checks, will the team merge it?
Probably not (https://prettier.io/docs/en/option-philosophy.html) but love the enthusiasm, and if we can find a solution everyone loves, would be happy to help guide a contribution!
@mike-marcacci curious what you think of this proposal?
@rattrayalex Re: adopting the same nested ternary syntax that we use for JSX, I think it's important that we consider the original motivation for that decision, which was that Prettier did not used to respect a ( followed by a \n. We used this all the time when writing JSX because elements typically span multiple lines, e.g.
error ? (
<div className="error">
<p>{error.message)</p>
</div>
) : (
<div>
<p>Everything's ok.</p>
</div>
)
When writing JSX, it's useful to spread out the elements on multiple lines so you can more easily see the nesting of the elements.
However, in the case of many JavaScript expressions, like the function calls you used in your example, you don't actually need multiple lines for those calls. So the extra ( + \n doesn't help code readability.
Indenting nested ternaries is a huge win for code readability IMO, and should be the default. If people don't like it, I'm fine with introducing an option for them to put all branches of the ternary at the same indentation level. But that's a change from how things used to be, so it should be opt-in and not the default.
FWIW, the main motivation between not indenting them all was to make switch-like expressions look good.
See also https://github.com/prettier/prettier/issues/737
Thanks for the link, @suchipi. Looks like there was a fork with a flattenTernaries option. Was that considered before making flattening the default behavior?
FWIW, here's the largest nested ternary that I've ever written, in the React Router codebase. With this change it goes from:
to this:
The code was already difficult to read, but this change makes it much more so.
Edit: e.g. consider the last trailing null. Which condition is that the else for?
@mjackson There's something strange going on in your screen shot. Prettier does not flatten ternaries that much:
Prettier 1.16.4 Playground link
--parser babylon
Input:
class Route extends React.Component {
render() {
return (
<RouterContext.Consumer>
{context => {
return (
<RouterContext.Provider value={props}>
{children && !isEmptyChildren(children)
? children
: props.match
? component
? React.createElement(component, props)
: render
? render(props)
: null
: null}
</RouterContext.Provider>
);
}}
</RouterContext.Consumer>
);
}
}
Output:
class Route extends React.Component {
render() {
return (
<RouterContext.Consumer>
{context => {
return (
<RouterContext.Provider value={props}>
{children && !isEmptyChildren(children)
? children
: props.match
? component
? React.createElement(component, props)
: render
? render(props)
: null
: null}
</RouterContext.Provider>
);
}}
</RouterContext.Consumer>
);
}
}
I have to admit I pinned several repositories I manage to old prettier versions, for exactly the same reason as @mjackson, complex expressions became really hard to read when this changed.
@lydell Thanks for the clarification, that's reasonable. I'd still prefer to have indentation at every level, but the new behavior isn't terrible.
Thanks. Also, I just randomly thought of another way to explain Prettier's formatting and wanted to share it. Flattening the tail is the else if of ternaries.
The thing that concerns me is that I don't think it's a purely stylistic preference. It erases the logical structure where one is implied. Reconstructing it from a flat list of punctuation marks where every "question" goes after : (but every ? means a truthy case) is pretty challenging unless you already know what the code is doing. Ternary is already very terse and frankly I was relying on its nesting structure a lot when reading. So it seems to me like it needs to be more strongly motivated than that it looks cleaner in some cases. I'm all for zero config btw. I just think in this case the cleanliness made comprehension significantly harder, and that makes the cleanliness not worth it to me. YMMV.
I do agree this shouldn't be configurable. I would personally prefer it be reverted to the old behavior.
@gaearon there's a lot of context in https://github.com/prettier/prettier/issues/737, but the idea here is that it's mirroring if/else if:
const StorybookLoader = ({ match }) =>
match.params.storyId === "button" ? (
<ButtonStorybook />
) : match.params.storyId === "color" ? (
<ColorBook />
) : match.params.storyId === "typography" ? (
<TypographyBook />
) : match.params.storyId === "loading" ? (
<LoaderStorybook />
) : match.params.storyId === "deal-list" ? (
<DealListStory />
) : (
<Message>
<Title>{"Missing story book"}</Title>
<Content>
<BackButton />
</Content>
</Message>
);
More concretely, a chain of flat else-if is easy to scan because words themselves help you differentiate conditions from inner expressions. You always know what's a condition.
A chain of ? : is already hard to interpret because ? foo from the next line is easy to read as a question ("if foo"). Removing nesting makes it super easy to make a mistake.
Example in https://github.com/prettier/prettier/issues/5814#issuecomment-468829133 is interesting. It doesn't suffer from this problem because the expressions all look the same. So it's obvious which is the condition and which is an inner expression.
Maybe we can somehow keep that pattern working with some heuristic, but revert for the less obvious pattern?
I would maybe keep the flat pattern but only if each item in the "conditions" list was a comparison (===, <, >). That makes it jump out and you can tell from a glance which is which. So if you want something like light switch, you can have it. But if you want a more complex chain, it gets nested.
FWIW, in JSX you want to use explicit conditions anyway. Since things like commentCount ? X : Y are ambiguous and often hide bugs. So that would work with that heuristic.
Maybe booleans and calls starting with is could be another exception.
FWIW, in JSX you want to use explicit conditions anyway. Since things like commentCount ? X : Y are ambiguous and often hide bugs.
Could you expound on this? I don't know what you mean.
I completely agree with @gaearon here:
The thing that concerns me is that I don't think it's a purely stylistic preference. It erases the logical structure where one is implied.
I have numerous examples of long ternary chains that rely heavily on indentation to communicate their logic. Further, the current implementation conditionally flattens logic chains, making it common for chains to be flattened differently at different depths. When this happens it's a nightmare to parse.
To make sure I'm not missing something, is there an argument for the current behavior that's still relevant, beside "it looks nicer in some very specific situations?" In my opinion, it's far more important to represent the logical structure clearly, even if it leads to extra lines or indents.