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

Indent `yield` at same level as `for` when using parentheses #592

Closed
olafurpg opened this Issue Nov 18, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@olafurpg
Member

olafurpg commented Nov 18, 2016

Scalafmt:

object Bar {
  for (x <- Nil)
    yield 2
}

I would like the input to look like this:

object Bar {
  for (x <- Nil)
  yield 2
}

From gitter: https://gitter.im/olafurpg/scalafmt?at=582ec607e51081951edf151b

for symmetry with if/else or try/catchs which seem to format like the former rather than the latter

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 18, 2016

Member

I'm struggling with this example

val sizes = for (row <- table)
yield
  (for (cell <- row)
  yield
    if (cell == null) 0
    else cell.toString.length)

How would you format that @lihaoyi?

Member

olafurpg commented Nov 18, 2016

I'm struggling with this example

val sizes = for (row <- table)
yield
  (for (cell <- row)
  yield
    if (cell == null) 0
    else cell.toString.length)

How would you format that @lihaoyi?

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 18, 2016

Member

Here's how it would format if I replace for/yield with if/else

[info]   val sizes =
[info]     if (row > table) a
[info]     else
[info]       (if (cell > row) b
[info]        else if (cell == null) 0
[info]        else cell.toString.length)

It could be the same for for/yield.

Member

olafurpg commented Nov 18, 2016

Here's how it would format if I replace for/yield with if/else

[info]   val sizes =
[info]     if (row > table) a
[info]     else
[info]       (if (cell > row) b
[info]        else if (cell == null) 0
[info]        else cell.toString.length)

It could be the same for for/yield.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 18, 2016

Member

Okay, so if the for/yield works exactly like if/else the output would be

[info]   val sizes =
[info]     for (row <- table)
[info]     yield
[info]       (for (cell <- row)
[info]        yield
[info]          if (cell == null) 0
[info]          else cell.toString.length)

I think this makes a lot of sense, I might want to make it the default behavior in 0.5.

Member

olafurpg commented Nov 18, 2016

Okay, so if the for/yield works exactly like if/else the output would be

[info]   val sizes =
[info]     for (row <- table)
[info]     yield
[info]       (for (cell <- row)
[info]        yield
[info]          if (cell == null) 0
[info]          else cell.toString.length)

I think this makes a lot of sense, I might want to make it the default behavior in 0.5.

olafurpg added a commit that referenced this issue Nov 18, 2016

@lihaoyi

This comment has been minimized.

Show comment
Hide comment
@lihaoyi

lihaoyi Nov 18, 2016

Yeah that looks good. Typically I wouldn't have a ( before the for, but instead surround the whole block with curlies:

[info]   val sizes =
[info]     for (row <- table)
[info]     yield {
[info]       for (cell <- row)
[info]       yield
[info]         if (cell == null) 0
[info]         else cell.toString.length
[info]     }   

But I assume scalafmt probably already does the right thing in such a scenario

lihaoyi commented Nov 18, 2016

Yeah that looks good. Typically I wouldn't have a ( before the for, but instead surround the whole block with curlies:

[info]   val sizes =
[info]     for (row <- table)
[info]     yield {
[info]       for (cell <- row)
[info]       yield
[info]         if (cell == null) 0
[info]         else cell.toString.length
[info]     }   

But I assume scalafmt probably already does the right thing in such a scenario

@lihaoyi

This comment has been minimized.

Show comment
Hide comment
@lihaoyi

lihaoyi Nov 18, 2016

Note that this is in addition to the "normal" way of doing fors:

[info]   val sizes =
[info]     for (row <- table) yield {
[info]       for (cell <- row)
[info]       yield
[info]         if (cell == null) 0
[info]         else cell.toString.length
[info]     }  

which hopefully should still work

lihaoyi commented Nov 18, 2016

Note that this is in addition to the "normal" way of doing fors:

[info]   val sizes =
[info]     for (row <- table) yield {
[info]       for (cell <- row)
[info]       yield
[info]         if (cell == null) 0
[info]         else cell.toString.length
[info]     }  

which hopefully should still work

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 18, 2016

Member

Hmmm. The "normal" way is not possible with the fix in #594. Instead, it follows the behavior of if/else

val sizes =
  for (row <- table)
  yield {
    for (cell <- row)
    yield
      if (cell == null) 0
      else cell.toString.length
  }

// yield stays on same line only when the body fits on single line
for (x <- Nil) yield { 1 }

However, I could update #594 to make if/else and for/yield also behave the same like this

if (cond) a else {
  // ...
}
for (a <- b) yield {
  // ....
}

I think it's reasonable to treat those patterns the same.

Member

olafurpg commented Nov 18, 2016

Hmmm. The "normal" way is not possible with the fix in #594. Instead, it follows the behavior of if/else

val sizes =
  for (row <- table)
  yield {
    for (cell <- row)
    yield
      if (cell == null) 0
      else cell.toString.length
  }

// yield stays on same line only when the body fits on single line
for (x <- Nil) yield { 1 }

However, I could update #594 to make if/else and for/yield also behave the same like this

if (cond) a else {
  // ...
}
for (a <- b) yield {
  // ....
}

I think it's reasonable to treat those patterns the same.

@sjrd

This comment has been minimized.

Show comment
Hide comment
@sjrd

sjrd Nov 18, 2016

I don't understand the rationale about symmetry with if/else and try/catch. In an if/else, else is on the same level of execution as if. Same for try/catch. In for/yield, yield is logically inside each iteration of for. Not after it or as an alternative to it, but inside. This is fundamentally different, IMO.

sjrd commented Nov 18, 2016

I don't understand the rationale about symmetry with if/else and try/catch. In an if/else, else is on the same level of execution as if. Same for try/catch. In for/yield, yield is logically inside each iteration of for. Not after it or as an alternative to it, but inside. This is fundamentally different, IMO.

@olafurpg

This comment has been minimized.

Show comment
Hide comment
@olafurpg

olafurpg Nov 18, 2016

Member

Okay, the behavior is now unchanged in this case

for (a <- b) yield {
  // ...
}

The full spec is here: https://github.com/olafurpg/scalafmt/pull/594/files#diff-a2424dd000145b09a8929364d3909db6

I'll merge the PR and we can iterate on this again after next release if there are still some quirks.

@sjrd It find indenting for/yield at the same level quite nice actually. However, I never really use for comprehensions with parentheses syntax, so this won't really affect me much.

Member

olafurpg commented Nov 18, 2016

Okay, the behavior is now unchanged in this case

for (a <- b) yield {
  // ...
}

The full spec is here: https://github.com/olafurpg/scalafmt/pull/594/files#diff-a2424dd000145b09a8929364d3909db6

I'll merge the PR and we can iterate on this again after next release if there are still some quirks.

@sjrd It find indenting for/yield at the same level quite nice actually. However, I never really use for comprehensions with parentheses syntax, so this won't really affect me much.

@olafurpg olafurpg closed this in 1bd2441 Nov 18, 2016

@olafurpg olafurpg changed the title from Indent `yield` at same level as `for` to Indent `yield` at same level as `for` when using parentheses Nov 18, 2016

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