Skip to content
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

Conditional OpenOp / CloseOp #556

Open
cgrushko opened this issue Dec 30, 2020 · 2 comments
Open

Conditional OpenOp / CloseOp #556

cgrushko opened this issue Dec 30, 2020 · 2 comments

Comments

@cgrushko
Copy link
Contributor

In ktfmt, I'd like to format the following:

fun longName() = coroutineScope {
  foo() 
  //
}

but if it doesn't fit in a single line, I'd like to get

fun longName() =
  coroutineScope {
    foo()
    //
  }

I only managed to create the first form by not wrapping the lambda in an OpenOp / CloseOp block, while the second form arises naturally from such a block around coroutine { … }.

One way to solve this is to create a conditional OpenOp / CloseOp, similarly to Indent.If, and then emit a block that materializes only if there had been a break after the =.

What do you think?

(I admit this isn't super clear, let me know if I should make another attempt at explaining this)

cc @cushon @bethcutler

cgrushko added a commit to cgrushko/ktfmt that referenced this issue Dec 30, 2020
Summary:
See examples in tests.

Note that the Google style version has a problem, because it has equal continuation and block indents. One solution would be to emit conditional blocks, after we create them :) (google/google-java-format#556)

Reviewed By: hick209

Differential Revision: D25738228

fbshipit-source-id: 98cbc5f622eccde635f07d5cecd9c93d40d6fd97
@cgrushko
Copy link
Contributor Author

Huh, looks like this can't be done, because an Op is used before the Indents are evaluated...

facebook-github-bot pushed a commit to facebook/ktfmt that referenced this issue Jan 4, 2021
Summary:
Pull Request resolved: #161

See examples in tests.

Note that the Google style version has a problem, because it has equal continuation and block indents. One solution would be to emit conditional blocks, after we create them :) (google/google-java-format#556)

Reviewed By: hick209

Differential Revision: D25738228

fbshipit-source-id: 4f803d17d507c0fe80482fe0a96b246119b3908f
facebook-github-bot pushed a commit to facebook/ktfmt that referenced this issue Jan 13, 2022
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 `lambdaIndentElse`.

## Change

I spent a lot of time trying to use boolean logic to simplify:
```
trailingDereferences && !hasTrailingLambda) || simple
```

...which didn't work. So then I thought about what's going on and realized it's equivalent to:
```
!hasTrailingLambda
```

I can't prove it using boolean logic, but I can prove it via test cases :)

 ---

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

Reviewed By: hick209

Differential Revision: D33485447

fbshipit-source-id: 9d8644c7c5173b22a559ff3e9e8e13f711126981
facebook-github-bot pushed a commit to facebook/ktfmt that referenced this issue Jan 13, 2022
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
@nreid260
Copy link
Contributor

nreid260 commented Feb 9, 2022

I think there would also be issues in generalizing conditional opens and closes. For example, all levels around the lambda would also need to be conditional, or else you'd still get the unwanted wrapping. The AST forming those levels could be arbitrarily deep.

I propose a different model for achieving this "block-like" formatting. I want two more Ops:

  1. AllowBlocklike: Specify a point in the token stream where a blocklike might begin, but isn't required to
  2. MarkBlocklikeHead: Specify where the "head" of a blocklike ends; all the stuff that should stay on the original line if it fits

These ops would be paired up, possibly using a tagging system like conditional indents use. The region marked by a pair would cut across multiple levels. MarkBlocklikeHead is optional, so AllowBlocklike is a no-op on its own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants