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

Router: indent trailing fewer braces, too #3514

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

kitbellew
Copy link
Collaborator

Previously, we applied a space indent when fewer braces were followed by another select. This takes care of the case when there are no more selects.

Towards #3489.

Previously, we applied a space indent when fewer braces were followed
by another select. This takes care of the case when there are no more
selects.
@soronpo
Copy link

soronpo commented Mar 16, 2023

@kitbellew Thank you so much for taking care of this! I really appreciate it.

@kitbellew kitbellew requested a review from tgodzik March 16, 2023 17:50
@kitbellew
Copy link
Collaborator Author

@kitbellew Thank you so much for taking care of this! I really appreciate it.

no problem. there will be another change proposed subsequently, to make some of it configurable. it still requires the compiler change proposed by Martin Odersky (and under review with Tomasz Godzik), as i didn't figure out a way to do this easily:

  // option 1
  foo:
    bar
  .baz
  .qux
  // option 2
  foo:
    bar
  .baz
    .qux

(and i already mentioned that i don't like what it looks like :) ).

>>>
object a:
def f(): Unit =
List(1, 2, 3).foldLeft(1):
case (a, b) => a + b
case (a, b) => a + b
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like way too much indentation. Looks like it's applied 3 times, wouldn't really one be enough? The alternative with {} will have single indentation:

  object a:
    def f(): Unit =
      List(1, 2, 3).foldLeft(1) {
        case (a, b) => a + b
        case (a, b) => a + b
      }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this example uses 4 as the main indent. so there's only one additional level added, to make way for a potential select continuation after it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't yet agreed on the dotty PR, no? I having doubts about whether we should even merge it. If we don't does this change still make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know. it fixes an inconsistency but you're right that both are not yet supported.

but if the dotty change isn't merged, we'll have a problem...

are you opposed to the change martin proposed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that change might hard to nail especially with the wierd 1-space rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i asked about that space as it seemed unexpected. no documentation mentions that one can be within one space of a significant indentation rather than matching it exactly... does the code not match it exactly?

if he agrees with the half-open interval you're proposing, would that unblock it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if he agrees with the half-open interval you're proposing, would that unblock it?

I think that would be the most logical yeah.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgodzik i would like to merge this change anyway. i have another pr ready, which makes this configurable, so if the compiler change is made, this is ready, and if not, there's a currently compatible alternative. wdyt?

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem to break anything, so it should be ok to merge. Sorry for not responding earlier, my last week was super hectic.

@kitbellew kitbellew merged commit 37b916a into scalameta:master Mar 28, 2023
@kitbellew kitbellew deleted the 3489_5 branch March 28, 2023 14:27
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

Successfully merging this pull request may close these issues.

3 participants