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

Update ktfmt to fix odd formatting with DSLs. #214

Closed
wants to merge 6 commits into from

Conversation

steventrouble
Copy link
Contributor

@steventrouble steventrouble commented Apr 22, 2021

Update KotlinInputAstVisitorBase.kt's binary expression handling to treat trailing lambdas as separate from the rest of the binary expression. While this should clean up some issues with binary expressions, the main intended side-effect was to fix DSLs:

#206

Before, it would format DSLs like this:

a =
  a {
    b =
      b {
        c = c
      }
  }

After, it should format them like this

a = a {
  b = b {
    c = c
  }
}

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 22, 2021
Copy link
Contributor

@cgrushko cgrushko left a comment

Choose a reason for hiding this comment

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

Thanks! this is awesome! and looks great.

  1. I'll run the modified code on our repo to see if it does anything weird. I'll report back.
  2. I commented about a test case that would show how line breaks affect this change. If you can think of additional ones it'll increase confidence in the change.

@facebook-github-bot
Copy link
Contributor

@cgrushko has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@cgrushko has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cgrushko
Copy link
Contributor

@steventrouble , thanks again for the PR and really sorry it took me forever to look at it.
I ran the modified ktfmt on our codebase, and it drastically improves a large number of lambdas - that's awesome :)

I ran into two weird results of the PR. I'll write a sanitized version of them ASAP and post here.
Meanwhile, I'll describe -

  1. a * b + c * d + e * f are broken into
    a * b +
    c * d +
    e * f
    This is inside an if, though there is a lambda later in the expression.

  2. foo.bar ?: 0 > 0 is broken into

foo.bar
  ?: 0 > 0

Can you take a look into why this might happen?

@cgrushko
Copy link
Contributor

Please hold on regarding my previous comment - I might have looked at the wrong diff, this might be an existing problem.

@facebook-github-bot
Copy link
Contributor

@cgrushko merged this pull request in 57a2a81.

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by da6615a34cbe9e62e1ce259bb1de146e03b652f3.

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by d4d46bb81b05d6fdb4d159ca2838725f5b4921a1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged Reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants