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

Fix double indentation in Elvis chains #416

Closed
wants to merge 1 commit into from

Conversation

nreid260
Copy link
Contributor

This is consistent with other binary operators, and prevents a strange second indent in Elvis chains.

@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 Aug 24, 2023
@nreid260
Copy link
Contributor Author

nreid260 commented Aug 24, 2023

@cgrushko

I feel this might be a contentious change. What I'm mostly hoping to fix is this output:

----------------------------------------
      foo =
        option1()
          ?: option2() ?: option3()
            ?: option4() ?: option5()
            ?: error()

It's possible to put Elvis at the start of the line, but it doesn't mix well with other operators.

@hick209
Copy link
Contributor

hick209 commented Aug 25, 2023

What were your expectations for that particular use case there?

Something like this maybe?

----------------------------------------
      foo =
        option1()
          ?: option2() ?: option3()
          ?: option4() ?: option5()
          ?: error()

@nreid260
Copy link
Contributor Author

nreid260 commented Aug 25, 2023

That proposed formatting is also acceptable to me. Although UNIFIED breaks might be preferable since the operators all have the same precedence:

---------------------------------------------------
      foo =
        option1()
          ?: option2()
          ?: option3()
          ?: option4()
          ?: option5()
          ?: error()

The approach in this PR was selected to in part because it made ?: consistent with with how other binary operator handled, which also makes future bugs less likely.

@nreid260
Copy link
Contributor Author

@hick209 @cgrushko @davidtorosyan Is there a strong opinion here? I'd like to move forward in some direction

@nreid260 nreid260 changed the title Move Elvis operator to end of broken lines Fix double indentation in Elvis chains Sep 13, 2023
@nreid260
Copy link
Contributor Author

I took another look and it was surprisingly easy to get the intended left-alignment. Hopefully that moves this from a decision to a bug fix.

@facebook-github-bot
Copy link
Contributor

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

@hick209
Copy link
Contributor

hick209 commented Sep 14, 2023

No strong opinions from my end other than "this looks like a nice improvement to me"

@nreid260
Copy link
Contributor Author

Great. I believe the latest version is uncontroversial as well.

@facebook-github-bot
Copy link
Contributor

@hick209 merged this pull request in ccc2b74.

lancethomps added a commit to lancethomps/ktfmt that referenced this pull request Oct 9, 2023
* upstream/main:
  Add unit tests to capture line break behavior on type specifiers
  Plugin doesn't work with if "Only VCS changed text" is selected from code-reformat settings (facebook#386)
  Bump version to 0.47-SNAPSHOT
  Bump version to 0.46
  Fix indentation of trailing comments in a bunch of cases (facebook#418)
  Adjust .editorconfig for kotlinlang style for IntelliJ to better align with ktfmt (facebook#412)
  Bump Kotlin version to 1.8.22
  Bump version to 0.46-SNAPSHOT
  Bump version to 0.45
  Bump word-wrap from 1.2.3 to 1.2.4 in /website (facebook#410)
  Use inExpression in a nullsafe way (facebook#417)
  Update ktfmt component on FBS:master
  Back out "Improve argsfile support"
  Improve argsfile support
  Fix double indentation in Elvis chains (facebook#416)
  Daily `arc lint --take KTFMT`
  Remove TypeNameClassifier
  Support context receivers (facebook#400)
  Added link to live playground directly into README file
  Keep imports from the same package if the name is overloaded (facebook#414)
@nreid260 nreid260 deleted the elvis branch April 23, 2024 06:10
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants