Skip to content

Commit

Permalink
Simplify argument indentation logic
Browse files Browse the repository at this point in the history
Summary:
## Context

After the changes in D33399531 (5164ad9), we were left with a few hard to understand variables which would get in the way of changing the newline logic. This aims to simplify the booleans behind `argsIndentElse`.

## Change

WARNING: Unlike previous diffs, this isn't a refactor - the final formatting is different (but for the better).

In order to simplify argument indentation logic, I changed:
```
!(trailingDereferences || simple)
```

To:
```
chunks.last().shouldKeepOnSameLine
```

These *aren't* the same, but they're very close - notice that only two (recently added) test cases are affected.

 ---

Also added a big comment explaining why we have this boolean. It's somewhat related to this github issue: [Conditional OpenOp / CloseOp · Issue #556](google/google-java-format#556)

Reviewed By: strulovich

Differential Revision: D33488425

fbshipit-source-id: a84b4716386909459f27383c4442216274d213b9
  • Loading branch information
davidtorosyan authored and facebook-github-bot committed Jan 13, 2022
1 parent f79e898 commit d74f46a
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -669,18 +669,6 @@ class KotlinInputAstVisitor(
// is the last expression a lambda? e.g.`foo.bar.apply { ... }`
val hasTrailingLambda = chunks.last().expressions.last().isLambda()

// Do we have expressions that should be split up following expressions that should be kept on
// the same line?
// e.g
// ```
// rainbow.red.orange.shine() // <-- chunk with shouldKeepOnSameLine = true
// .yellow // <--|
// .green // <--|
// .blue // <--|-- chunk with shouldKeepOnSameLine = false
// ```
val trailingDereferences =
chunks.first().shouldKeepOnSameLine && !chunks.last().shouldKeepOnSameLine

// Do we have any expressions that should be kept on the same line?
// True example: `rainbow.red.orange.shine()`
// False example: `rainbow.red.orange`
Expand All @@ -691,8 +679,31 @@ class KotlinInputAstVisitor(
// simple chain with no meaningful groups or trailing lambda, e.g `rainbow.red.orange`
val simple = !hasPrefixes && !hasTrailingLambda

// how to indent function arguments if the line is not broken
val argsIndentElse = if (trailingDereferences || simple) expressionBreakIndent else ZERO
// When the last chunk is meant to be on one line, reduce the indentation of arguments:
// ```
// rainbow.shine(
// infrared,
// ultraviolet,
// )
// ```
// Here's an example of the line being broken, so we don't reduce the indentation:
// ```
// rainbow.red.orange.yellow
// .shine(
// infrared,
// ultraviolet,
// )
// ```
// Here's a negative side effect, can't be fixed unless we can detect that the invocation is not
// on the same line as the first reference (not currently possible):
// ```
// rainbow.red.orange.yellow
// .key.shine(
// infrared,
// ultraviolet,
// )
// ```
val argsIndentElse = if (chunks.last().shouldKeepOnSameLine) ZERO else expressionBreakIndent

// When we have a trailing lambda and the line it's on isn't broken, reduce its indentation:
// ```
Expand Down
8 changes: 4 additions & 4 deletions core/src/test/java/com/facebook/ktfmt/format/FormatterTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4958,7 +4958,7 @@ class FormatterTest {
"""
|---------------------
|getRainbow(
| aa, bb, cc)
| aa, bb, cc)
| .z { it }
|""".trimMargin(),
deduceMaxWidth = true)
Expand Down Expand Up @@ -5212,9 +5212,9 @@ class FormatterTest {
"""
|-------------------------
|getRainbow(
| infrared,
| ultraviolet,
|)
| infrared,
| ultraviolet,
| )
| .z { it }
|""".trimMargin(),
deduceMaxWidth = true)
Expand Down

0 comments on commit d74f46a

Please sign in to comment.