Flatten nested ternaries #1171

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@vjeux
Collaborator

vjeux commented Apr 9, 2017

This keeps coming up that the way prettier prints nested ternaries is not good. Here's an attempt at solving the situation. A good compromise is to

  • keep printing ternaries the way they are but do not indent nested ones
  • have a single group for chained ternaries, this way the last ones also break instead of awkwardly being fitting in a single line

The implementation is pretty straightforward, just look at the parent and print it differently if it's a ternary and we are in the second group.

Before:

const value = condition1
  ? value1
  : condition2 ? value2 : condition3 ? value3 : value4;

After:

const value = condition1
  ? value1
  : condition2
  ? value2
  : condition3
  ? value3
  : value4;

Fixes #737

Flatten nested ternaries
This keeps coming up that the way prettier prints nested ternaries is not good. Here's an attempt at solving the situation. A good compromise is to
- keep printing ternaries the way they are but do not indent nested ones
- have a single group for chained ternaries, this way the last ones also break instead of awkwardly being fitting in a single line

The implementation is pretty straightforward, just look at the parent and print it differently if it's a ternary and we are in the second group.

Fixes #737
@mvolkmann

This comment has been minimized.

Show comment
Hide comment
@mvolkmann

mvolkmann Apr 9, 2017

This is SO much better than the current formatting.

This is SO much better than the current formatting.

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Apr 10, 2017

Contributor

Why not indent nested? It's a huge improvement over what's there now, of course 😄

Contributor

SimenB commented Apr 10, 2017

Why not indent nested? It's a huge improvement over what's there now, of course 😄

@CiGit CiGit referenced this pull request in prettier/prettier-vscode Apr 10, 2017

Closed

Nested ternary operations #66

@mvolkmann

This comment has been minimized.

Show comment
Hide comment
@mvolkmann

mvolkmann Apr 10, 2017

I'm confused, @SimenB. It indents nested ternaries now. Each one is indented more than the previous one. If there is more than one ternary, it looks like code I've never seen a person write. It becomes deeply nested fast. I think it's fair to say that a side goal of the current formatting is to discourage use of nested ternaries. I think the main issue is whether that is a good goal. Me and several others think it is possible to format nested ternaries in a way that looks nice and is not confusing.

I'm confused, @SimenB. It indents nested ternaries now. Each one is indented more than the previous one. If there is more than one ternary, it looks like code I've never seen a person write. It becomes deeply nested fast. I think it's fair to say that a side goal of the current formatting is to discourage use of nested ternaries. I think the main issue is whether that is a good goal. Me and several others think it is possible to format nested ternaries in a way that looks nice and is not confusing.

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Apr 10, 2017

Contributor

I find

const value = condition1
  ? value1
  : condition2
    ? value2
    : condition3
      ? value3
      : value4;

to be easier to follow.

Of course, I haven't written a nested ternary for a couple of years (eslint would tell me off), so this doesn't really impact me 😄

Contributor

SimenB commented Apr 10, 2017

I find

const value = condition1
  ? value1
  : condition2
    ? value2
    : condition3
      ? value3
      : value4;

to be easier to follow.

Of course, I haven't written a nested ternary for a couple of years (eslint would tell me off), so this doesn't really impact me 😄

@mvolkmann

This comment has been minimized.

Show comment
Hide comment
@mvolkmann

mvolkmann Apr 10, 2017

That's not exactly how Prettier does it though. It indents more than that. Many lines get indented four spaces instead of two. Your example looks fine for three ternaries, but not for say seven.

That's not exactly how Prettier does it though. It indents more than that. Many lines get indented four spaces instead of two. Your example looks fine for three ternaries, but not for say seven.

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Apr 10, 2017

Contributor

7 nested ternaries should really be refactored anyways, that's really unreadable :P

Contributor

SimenB commented Apr 10, 2017

7 nested ternaries should really be refactored anyways, that's really unreadable :P

@mvolkmann

This comment has been minimized.

Show comment
Hide comment
@mvolkmann

mvolkmann Apr 10, 2017

I just don't understand how anyone could claim it is unreadable when written like this:

const value =
  condition1 ? value1 :
  condition2 ? value2 :
  condition3 ? value3 :
  condition4 ? value4 :
  condition5 ? value5 :
  condition6 ? value6 :
  condition7 ? value7 :
  value8;

But everyone is entitled to their opinion and I've already lost this battle.

I just don't understand how anyone could claim it is unreadable when written like this:

const value =
  condition1 ? value1 :
  condition2 ? value2 :
  condition3 ? value3 :
  condition4 ? value4 :
  condition5 ? value5 :
  condition6 ? value6 :
  condition7 ? value7 :
  value8;

But everyone is entitled to their opinion and I've already lost this battle.

@jlongster

This comment has been minimized.

Show comment
Hide comment
@jlongster

jlongster Apr 11, 2017

Member

One of the things to think about here is how it looks when the expression (between : or ?) is broken up across multiple lines. If we're going to change this (and I think we should) it might be nice to go ahead and explore this syntax as well:

foo ? (
  bigFunctionCall(
    arg1,
    arg2
  )
) : bar ? (
  // some other big expr
) : baz ? (
  // something else
)

Or some variant, where we use parentheses to aid making it more structured since the expressions are bigger.

This could possible work with @mvolkmann's syntax, but we move the value inside parens if it breaks across multiple lines.

(Edit: We'd also make @ryanflorence's week, but it probably would require us to eagerly group up conditional expressions like we do member expressions and make it more complicated)

Member

jlongster commented Apr 11, 2017

One of the things to think about here is how it looks when the expression (between : or ?) is broken up across multiple lines. If we're going to change this (and I think we should) it might be nice to go ahead and explore this syntax as well:

foo ? (
  bigFunctionCall(
    arg1,
    arg2
  )
) : bar ? (
  // some other big expr
) : baz ? (
  // something else
)

Or some variant, where we use parentheses to aid making it more structured since the expressions are bigger.

This could possible work with @mvolkmann's syntax, but we move the value inside parens if it breaks across multiple lines.

(Edit: We'd also make @ryanflorence's week, but it probably would require us to eagerly group up conditional expressions like we do member expressions and make it more complicated)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Apr 11, 2017

Collaborator

@jlongster would you print it as

foo ? (
  bigFunctionCall(
    arg1,
    arg2
  )
) : bar

for a single ternary in order to be consistent?

Collaborator

vjeux commented Apr 11, 2017

@jlongster would you print it as

foo ? (
  bigFunctionCall(
    arg1,
    arg2
  )
) : bar

for a single ternary in order to be consistent?

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Apr 13, 2017

I think what would be helpful to the discussion would be to avoid using easily-aligned examples. Make it messy so you're not picking something because the example case lines up nicely. Here's one way to make it look not so pretty, so you're working from the practical case:

const avatarURL = user.avatar && user.avatar.url
  ? user.avatar.url
  : user.name !== null
  ? generateAvatar(
    user.name.first,
    user.name.last
  )
  : generateAvatar(user.id)

That might be one way it comes out.

Personally, I find that a little harder to read than other forms. It's better than the current behavior, but I think it can be improved with indentation. Here's @SimenB's case:

const avatarURL = user.avatar && user.avatar.url
  ? user.avatar.url
  : user.name !== null
    ? generateAvatar(
      user.name.first,
      user.name.last
    )
    : generateAvatar(user.id)

And @mvolkmann's:

const avatarURL = 
  user.avatar && user.avatar.url ? user.avatar.url : 
  user.name !== null ? generateAvatar(
    user.name.first,
    user.name.last
  ) : generateAvatar(user.id)

Thoughts?

timdorr commented Apr 13, 2017

I think what would be helpful to the discussion would be to avoid using easily-aligned examples. Make it messy so you're not picking something because the example case lines up nicely. Here's one way to make it look not so pretty, so you're working from the practical case:

const avatarURL = user.avatar && user.avatar.url
  ? user.avatar.url
  : user.name !== null
  ? generateAvatar(
    user.name.first,
    user.name.last
  )
  : generateAvatar(user.id)

That might be one way it comes out.

Personally, I find that a little harder to read than other forms. It's better than the current behavior, but I think it can be improved with indentation. Here's @SimenB's case:

const avatarURL = user.avatar && user.avatar.url
  ? user.avatar.url
  : user.name !== null
    ? generateAvatar(
      user.name.first,
      user.name.last
    )
    : generateAvatar(user.id)

And @mvolkmann's:

const avatarURL = 
  user.avatar && user.avatar.url ? user.avatar.url : 
  user.name !== null ? generateAvatar(
    user.name.first,
    user.name.last
  ) : generateAvatar(user.id)

Thoughts?

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Apr 13, 2017

Contributor

Out of those, I find the middle one the most readable. But again, I haven't read/written nested ternaries in a while, as airbnb style enforces no nesting, so that might just be me being "unfamiliar" with the syntax.

Contributor

SimenB commented Apr 13, 2017

Out of those, I find the middle one the most readable. But again, I haven't read/written nested ternaries in a while, as airbnb style enforces no nesting, so that might just be me being "unfamiliar" with the syntax.

@kimjoar

This comment has been minimized.

Show comment
Hide comment
@kimjoar

kimjoar Apr 13, 2017

Contributor

Agreed on the middle being the most readable.

Contributor

kimjoar commented Apr 13, 2017

Agreed on the middle being the most readable.

@ianstormtaylor

This comment has been minimized.

Show comment
Hide comment
@ianstormtaylor

ianstormtaylor Apr 13, 2017

I agree with the middle syntax there, that's how I would write those to be most readable later. (Although I agree, I'd avoid it generally.)

I agree with the middle syntax there, that's how I would write those to be most readable later. (Although I agree, I'd avoid it generally.)

@mvolkmann

This comment has been minimized.

Show comment
Hide comment
@mvolkmann

mvolkmann Apr 13, 2017

Good idea to consider a more complicated case! Here is how I would write it:

const avatarURL = 
  user.avatar && user.avatar.url ? user.avatar.url : 
  user.name !== null ?
    generateAvatar(
      user.name.first,
      user.name.last
    ) :
  generateAvatar(user.id)

The rule here would be that a value can only be on the same line as its condition if it fits within 80 columns. Otherwise the whole value is moved to the next line and indented.

The only thing I don't prefer about the @SimenB's case is that each ternary gets nested deeper than than the one before. That's the primary thing I'd like to avoid. The first example and mine do avoid that.

Good idea to consider a more complicated case! Here is how I would write it:

const avatarURL = 
  user.avatar && user.avatar.url ? user.avatar.url : 
  user.name !== null ?
    generateAvatar(
      user.name.first,
      user.name.last
    ) :
  generateAvatar(user.id)

The rule here would be that a value can only be on the same line as its condition if it fits within 80 columns. Otherwise the whole value is moved to the next line and indented.

The only thing I don't prefer about the @SimenB's case is that each ternary gets nested deeper than than the one before. That's the primary thing I'd like to avoid. The first example and mine do avoid that.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Apr 13, 2017

I'd write:

const avatarURL = user.avatar && user.avatar.url
  ? user.avatar.url
  : user.name !== null
  ? generateAvatar(
      user.name.first,
      user.name.last
    )
  : generateAvatar(user.id);

Or

const avatarURL = (
  user.avatar && user.avatar.url
  ? user.avatar.url
  : user.name !== null
  ? generateAvatar(
      user.name.first,
      user.name.last
    )
  : generateAvatar(user.id)
);

I prefer the latter but I can accept the former.

I'd write:

const avatarURL = user.avatar && user.avatar.url
  ? user.avatar.url
  : user.name !== null
  ? generateAvatar(
      user.name.first,
      user.name.last
    )
  : generateAvatar(user.id);

Or

const avatarURL = (
  user.avatar && user.avatar.url
  ? user.avatar.url
  : user.name !== null
  ? generateAvatar(
      user.name.first,
      user.name.last
    )
  : generateAvatar(user.id)
);

I prefer the latter but I can accept the former.

@tlrobinson

This comment has been minimized.

Show comment
Hide comment
@tlrobinson

tlrobinson Apr 13, 2017

We use this pattern a lot in JSX:

<div>
  { conditionA ?
    <div>foo</div>
  : conditionB ?
    <div>
      <div>bar</div>
    </div>
  : null }
</div>

I think that's similar to @jlongster's suggestion here #1171 (comment)

This would be ok (but not as readable IMHO):

<div>
  { conditionA
  ? <div>foo</div>
  : conditionB
  ? <div>foo</div>
  : null }
</div>

...but would start looking weird with multilines.

I guess something like this would be ok:

<div>
  { conditionA
  ? <div>foo</div>
  : conditionB
  ?
    <div>
      <div>bar</div>
    </div>
  : null }
</div>

Just my 2¢!

BTW, this is the only thing holding us back from prettierifying our entire codebase, awesome tool otherwise!

tlrobinson commented Apr 13, 2017

We use this pattern a lot in JSX:

<div>
  { conditionA ?
    <div>foo</div>
  : conditionB ?
    <div>
      <div>bar</div>
    </div>
  : null }
</div>

I think that's similar to @jlongster's suggestion here #1171 (comment)

This would be ok (but not as readable IMHO):

<div>
  { conditionA
  ? <div>foo</div>
  : conditionB
  ? <div>foo</div>
  : null }
</div>

...but would start looking weird with multilines.

I guess something like this would be ok:

<div>
  { conditionA
  ? <div>foo</div>
  : conditionB
  ?
    <div>
      <div>bar</div>
    </div>
  : null }
</div>

Just my 2¢!

BTW, this is the only thing holding us back from prettierifying our entire codebase, awesome tool otherwise!

@ianstormtaylor

This comment has been minimized.

Show comment
Hide comment
@ianstormtaylor

ianstormtaylor Apr 14, 2017

The JSX case is actually interesting, because it's (one of?) the only places where the ternary is required to keep things inline easily, and where you actually might not want nesting because it adds confusion to the meaning of the output. But I'm not sure personally that it feels like a common enough case to merit weird behavior.

Otherwise, for all of the pure JS cases, I think not nesting is purely a stylistic choice that should be ignored in favor of being consistent with nesting in other places. (Often just a "what, why?!" choice.)

(That said: I don't actually care what style Prettier chooses for these kinds of nested ternaries because they feel like huge edge cases to me, so please ignore everything I say!)

The JSX case is actually interesting, because it's (one of?) the only places where the ternary is required to keep things inline easily, and where you actually might not want nesting because it adds confusion to the meaning of the output. But I'm not sure personally that it feels like a common enough case to merit weird behavior.

Otherwise, for all of the pure JS cases, I think not nesting is purely a stylistic choice that should be ignored in favor of being consistent with nesting in other places. (Often just a "what, why?!" choice.)

(That said: I don't actually care what style Prettier chooses for these kinds of nested ternaries because they feel like huge edge cases to me, so please ignore everything I say!)

@kentor

This comment has been minimized.

Show comment
Hide comment
@kentor

kentor Apr 14, 2017

I'd like the formatting to parallel if/else statements

Suppose ifs were expressions instead of statements:

<div>
  {if (condition) {
    <A />
  } else if (condition2) {
    <B />
  } else {
    null
  }}
</div>

With ternaries this would be

<div>
  {condition ?
    <A />
  : condition2 ?
    <B />
  :
    null
  }
</div>

That is the most readable to me. This is similar to @ryanflorence's style but without the unnecessary parentheses.

The issue with hugging the ? right now is that the jsx tags are indented way too far to the right. With 80 char line limit this does not leave a lot left.

This is also holding me back from adopting prettier at the moment.

kentor commented Apr 14, 2017

I'd like the formatting to parallel if/else statements

Suppose ifs were expressions instead of statements:

<div>
  {if (condition) {
    <A />
  } else if (condition2) {
    <B />
  } else {
    null
  }}
</div>

With ternaries this would be

<div>
  {condition ?
    <A />
  : condition2 ?
    <B />
  :
    null
  }
</div>

That is the most readable to me. This is similar to @ryanflorence's style but without the unnecessary parentheses.

The issue with hugging the ? right now is that the jsx tags are indented way too far to the right. With 80 char line limit this does not leave a lot left.

This is also holding me back from adopting prettier at the moment.

@bakkot

This comment has been minimized.

Show comment
Hide comment
@bakkot

bakkot Apr 14, 2017

Collaborator

@kentor, there's an important difference from if/else, which is that it's not obvious whether the thing after : is a condition or result until the end of the line (or the next line). I think the increasing-indent style (@SimonB's) is the most readable, for this reason.

I also feel the increasing-indent style is the most consistent with how prettier formats expressions more generally, which seems like it ought to be a guiding concern for edge cases like this.

Collaborator

bakkot commented Apr 14, 2017

@kentor, there's an important difference from if/else, which is that it's not obvious whether the thing after : is a condition or result until the end of the line (or the next line). I think the increasing-indent style (@SimonB's) is the most readable, for this reason.

I also feel the increasing-indent style is the most consistent with how prettier formats expressions more generally, which seems like it ought to be a guiding concern for edge cases like this.

@mvolkmann

This comment has been minimized.

Show comment
Hide comment
@mvolkmann

mvolkmann Apr 17, 2017

@umidbekkarimov

This comment has been minimized.

Show comment
Hide comment
@umidbekkarimov

umidbekkarimov Apr 17, 2017

Contributor

consecutive :'s. Typically those alternate. How does it make sense for a ? to follow FlatButton JSX?

Sorry @mvolkmann, I gave bad example there (i'm not good in ternaries) and removed this comment but forgot to provide a better example:

How it looks now:

return !props.id
  ? <div>Invalid ID: {props.id}</div>
  : props.fetching
      ? <div>Loading ...</div>
      : props.notFound
          ? <div>
              Not Found: {props.id}
            </div>
          : <div>{props.name}</div>;

In this case readability would be same, but only with one line conditions:

return
  !props.id ? <div>Invalid ID: {props.id}</div> :
  props.fetching ? <div>Loading ...</div> :
  props.notFound ? (
      <div>Not Found: {props.id}</div>
  ) : <div>{props.name}</div>;

But this one can confuse.

return !props.id
  ? <div>Invalid ID: {props.id}</div>
  : props.fetching
  ? <div>Loading ...</div>
  : props.notFound
  ? <div>
      Not Found: {props.id}
    </div>
  : <div>{props.name}</div>;
Contributor

umidbekkarimov commented Apr 17, 2017

consecutive :'s. Typically those alternate. How does it make sense for a ? to follow FlatButton JSX?

Sorry @mvolkmann, I gave bad example there (i'm not good in ternaries) and removed this comment but forgot to provide a better example:

How it looks now:

return !props.id
  ? <div>Invalid ID: {props.id}</div>
  : props.fetching
      ? <div>Loading ...</div>
      : props.notFound
          ? <div>
              Not Found: {props.id}
            </div>
          : <div>{props.name}</div>;

In this case readability would be same, but only with one line conditions:

return
  !props.id ? <div>Invalid ID: {props.id}</div> :
  props.fetching ? <div>Loading ...</div> :
  props.notFound ? (
      <div>Not Found: {props.id}</div>
  ) : <div>{props.name}</div>;

But this one can confuse.

return !props.id
  ? <div>Invalid ID: {props.id}</div>
  : props.fetching
  ? <div>Loading ...</div>
  : props.notFound
  ? <div>
      Not Found: {props.id}
    </div>
  : <div>{props.name}</div>;
@nmn

This comment has been minimized.

Show comment
Hide comment
@nmn

nmn Apr 21, 2017

  1. Prettier needs to handle nested ternaries regardless of developer's opinion of whether they should be used. They are also pretty common within JSX because of the lack of an if expression.
  2. The original solution by @vjeux works great.
  3. It's also the formatting championed by Standard and it's many variants.
  4. Multiple indentation is less scalable, as you could easily run over the column character limit.

In Conclusion,
Let's Merge This!

nmn commented Apr 21, 2017

  1. Prettier needs to handle nested ternaries regardless of developer's opinion of whether they should be used. They are also pretty common within JSX because of the lack of an if expression.
  2. The original solution by @vjeux works great.
  3. It's also the formatting championed by Standard and it's many variants.
  4. Multiple indentation is less scalable, as you could easily run over the column character limit.

In Conclusion,
Let's Merge This!

@azz

This comment has been minimized.

Show comment
Hide comment
@azz

azz Apr 26, 2017

Member

@kentor I hope do { } makes it into the spec so we can forget that nested ternaries are a thing 😞.

<div>
  {do { 
    if (condition) {
      <A />
    } else if (condition2) {
      <B />
    } else {
      null
    }
  }}
</div>
Member

azz commented Apr 26, 2017

@kentor I hope do { } makes it into the spec so we can forget that nested ternaries are a thing 😞.

<div>
  {do { 
    if (condition) {
      <A />
    } else if (condition2) {
      <B />
    } else {
      null
    }
  }}
</div>
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux May 5, 2017

Collaborator

I'm doing a cleanup of all the pull requests, I'm going to close this one. We still want to figure out what to do with nested ternaries though, so the discussion is not over yet.

Collaborator

vjeux commented May 5, 2017

I'm doing a cleanup of all the pull requests, I'm going to close this one. We still want to figure out what to do with nested ternaries though, so the discussion is not over yet.

@vjeux vjeux closed this May 5, 2017

scotchfield added a commit to Khan/perseus that referenced this pull request Jul 10, 2017

Removes eslint-ignore and runs prettier with some custom formatting
Summary:
This commit runs prettier over the rest of our Perseus components
directory and removes the bulk eslint-ignore warnings for style
issues.

`prettier --tab-width 4 --trailing-comma es5 --bracket-spacing false`

Again, it's pretty good, but not perfect! The single fix I've had to
address manually (except for console warnings and real lint issues
like props and React ordering) is the ternary formatting. Seems
like the prettier team is aware of this:

prettier/prettier#1171

Otherwise, it's a lot of `var` to `let` or `const`. :)

Test Plan:
* `ka-lint`
* `make test`
* Test in a browser

Reviewers: emily

Reviewed By: emily

Subscribers: jared, kevinb, john

Differential Revision: https://phabricator.khanacademy.org/D37087
@reywright

This comment has been minimized.

Show comment
Hide comment
@reywright

reywright Nov 17, 2017

I'm a big fan of a lot of these. I think anything is a huge improvement over how ternaries format now. A lot of good suggestions in this thread.

I'm a big fan of a lot of these. I think anything is a huge improvement over how ternaries format now. A lot of good suggestions in this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment