Router: handle if with/without braces consistently

[kitbellew]

To do that, allow space only if 'else' is empty or the entire expression fits on the line.

Fixes #1747.

scala-repos diffs: kitbellew/scala-repos@3670a38?w=1

p.s. old diffs: kitbellew/scala-repos@dc650f9?w=1

[kitbellew]

@olafurpg @tanishiking @poslegm PTAL

[poslegm]

Thank you, I like it! But I would rather wait for the rest maintainers opinion because of large diffs in scala-repos

[kitbellew]

Thank you, I like it! But I would rather wait for the rest maintainers opinion because of large diffs in scala-repos

@olafurpg @tanishiking your thoughts on this change? the diffs in scala-repos change

  val a = if (b) {
    c
  } else d

to

  val a =
    if (b) {
      c
    } else d

for consistency with existing

  val a =
    if (b)
      c
    else d

[tanishiking]

Thanks! I'll take a look this evening :)

[tanishiking]

Thank you so much for working on this @kitbellew 👍
IMO, I don't like this formatting result, and judging from the original scala-repos coding styles, I guess there will be a backlash against this change, because a lot of codebases try to accommodate if statement to the same line with LHS.

To fix the inconsistency mentioned in #1747, I think it would me better to format like the following examples.

// 3
val ok3 = if (a > 10)
  Some(a)
else
  None

// to
val ko3 = if (a > 10)
    Some(a)
  else
    None

(personally, it's best to "val ko3 = if (a > 10) Some(a) else None" in a single line)

// 4
val ok4 = if (a > 10) Some(a) else {
  None
}

// to
val ok4 = if (a > 10)
    Some(a)
  else {
    None
  }

or something. (Personally I'm ok with the current scalafmt's behavior, and we don't necessarily change the current behavior if someone is REALLY want to change current behavior).

What do you think about this? @paulkore

[kitbellew]

@tanishiking @poslegm PTAL, updated code with Rikito's suggestion, uploaded new diffs (mostly whitespace for indent, and some non-brace ifs looking more like brace ifs).

[poslegm]

I don't sure that additional indent is the best solution for the consistency problem.

Especially I don't like this:

val x = if (x > max) "a"
  else "b"

Personally I prefer first version of @kitbellew's changes because it perfectly standardizes if/else formatting. But because of large diff in real-world codebases we may be forced to keep current "liberal" behavior 😞

[kitbellew]

Especially I don't like this:

val x = if (x > max) "a"
  else "b"

Neither do I (including the brace version). Continuing without a break logically means that there's only one expression on the right, which works with a def returning a Unit, a match, or a case guard; but it reads poorly in assignment which requires both then and else (i.e., two expressions).

Abandon? Add a parameter similar to newlines.alwaysBeforeMultilineDef or alwaysBeforeElseAfterCurlyIf, whichever one looks more appropriate? Generally, I was planning to handle such cases in my #1627 which I will start submitting soon anyway.

[poslegm]

It really looks like a good case for newlines.* configuration. I am ok with new setting for this.

[kitbellew]

It really looks like a good case for newlines.* configuration. I am ok with new setting for this.

should we simply reuse alwaysBeforeMultilineDef with an edition guard? this setting seems quite similar in nature, especially if we interpret Def as Defn (which covers val, var etc.). we've done it before with alwaysBeforeElseAfterCurlyIf as applied to try-finally.

[poslegm]

should we simply reuse alwaysBeforeMultilineDef with an edition guard?

sounds reasonable

[paulkore]

Hi all, I'm glad to see that progress is being made on this issue!

@tanishiking , to answer your question from before, I agree with you:
Placing the "if" on the same line as the val assignment seems to be standard practice in languages where "if/else" has a return value.

However, I'm not sure about the indentation.
Based on the codebases that I've worked in, it seems more popular to keep the same indentation in if/else, regardless of whether it's used procedurally or in an assignment:

// procedural
if (a > 10) {
  println("A")
} else {
  println("B")
}

// assignment
val x = if (a > 10) {
  Some(a)
} else {
  None
}

[tanishiking]

Thank you so much for modifying @kitbellew and your comment @paulkore

IMO, we don't have to fix this "inconsistency" between if/else with and without braces. (I mean it's best to maintain the status quo).

Though @paulkore mentioned that there's an inconsistency between those formatting results, I think we don't have to honor the consistency about the indent/newline between them,
because they have different syntax, and actually (as far as I know) a lot of scala developers prefer to use different coding styles for if/else statement with and without braces.

// without braces
val ok3 =
  if (a > 10) Some(a)
  else None

// with braces
val ok3 = if (a > 10) {
  Some(a)
} else {
  None
}

That's why I believe it's better not to change the current behavior.
It might be better to hear more opinions from other scala developers, I guess.

---

It's ok with adding an option, but in that case, I think it's better to keep the default behavior.