Router: handle then/else/do/try/finally with optional braces
#2415
Conversation
then/else with optional bracesthen/else/do/try/finally with optional braces
scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala
Outdated
Show resolved
Hide resolved
scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala
Outdated
Show resolved
Hide resolved
scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala
Outdated
Show resolved
Hide resolved
scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala
Outdated
Show resolved
Hide resolved
scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatOps.scala
Outdated
Show resolved
Hide resolved
| } | ||
| t.thenp match { | ||
| case x: Term.If => nestedIf(x) | ||
| case Term.Block(List(x: Term.If)) => nestedIf(x) |
There was a problem hiding this comment.
| case Term.Block(List(x: Term.If)) => nestedIf(x) | |
| case Term.Block((x: Term.If) :: _) => nestedIf(x) |
?
I don't think we can have an indented block with a single if currently. And would that not be covered by the next case?
There was a problem hiding this comment.
Also, what was the issue with nested ifs? It seems it should work normally 🤔
There was a problem hiding this comment.
this covers a block which contains only an if, not one which simply starts with an if.
if the block has multiple statements, a break is a must.
if the block only has an if, you must break, too, if it doesn't have an else but the outer one does:
if a then if b then c
else d
this else could belong to either of them since their indent is the same.
| def unapply( | ||
| ftMeta: FormatToken.Meta | ||
| )(implicit style: ScalafmtConfig): Option[OptionalBraces] = | ||
| getMissingBrace(ftMeta).flatMap { nft => |
There was a problem hiding this comment.
It seems that we need to check getMissingBrace(ftMeta) on each of the new unapplies, which makes us do it over and over again. I wonder whether we should do it all in a single OptionalBraces object? Looks like we should be able to merge that. Or am I too worried about performance impact that does not exist there?
There was a problem hiding this comment.
good comment. i think it's time to go back to a single matcher. i will send an update in the morning.
| bar | ||
| >>> | ||
| object a: | ||
| if (a) then foo |
There was a problem hiding this comment.
Should we also test nested if (a)?
| } | ||
| t.thenp match { | ||
| case x: Term.If => nestedIf(x) | ||
| case Term.Block(List(x: Term.If)) => nestedIf(x) |
There was a problem hiding this comment.
this covers a block which contains only an if, not one which simply starts with an if.
if the block has multiple statements, a break is a must.
if the block only has an if, you must break, too, if it doesn't have an else but the outer one does:
if a then if b then c
else d
this else could belong to either of them since their indent is the same.
| def unapply( | ||
| ftMeta: FormatToken.Meta | ||
| )(implicit style: ScalafmtConfig): Option[OptionalBraces] = | ||
| getMissingBrace(ftMeta).flatMap { nft => |
There was a problem hiding this comment.
good comment. i think it's time to go back to a single matcher. i will send an update in the morning.
| bar | ||
| >>> | ||
| object a: | ||
| if (a) then foo |
There are number of common things they do, before they potentially fail, so it makes sense to do that in one rule. Also, makes code management a lot simpler.
While normally the new syntax replaces parentheses in `if` and `while` with explicit keywords `then` and `do`, respectively, it's not really an error to keep parentheses, so let's handle that case.
Also, abide by
newlines.sourcefor optional braces.