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

Don't wrap method declaration #1416

Merged
merged 4 commits into from
Jun 16, 2020
Merged

Conversation

hesselink
Copy link
Contributor

Before this PR

The default wrapping for long method declarations wrapped on the method modifiers, return type and name instead of the parameters, as most people on CRs seem to prefer.

After this PR

==COMMIT_MSG==
Wrap method declarations on parameters, not on modifiers and return type.
==COMMIT_MSG==

Possible downsides?

My eclipse saved in a higher format so that caused a bunch of changes. I read through these and tried to match to what was done before, but I could have missed something.

@hesselink hesselink requested review from gatesn and carterkozak June 16, 2020 12:33
@hesselink hesselink self-assigned this Jun 16, 2020
@changelog-app
Copy link

changelog-app bot commented Jun 16, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

When using Eclipse's built-in formatter, method declarations are now wrapped on parameters, not on modifiers and return type.

Check the box to generate changelog(s)

  • Generate changelog entry

@hesselink hesselink changed the title Dont wrap method declaration Don't wrap method declaration Jun 16, 2020
@iamdanfox
Copy link
Contributor

The thing that sucks here is that the eclipse and intellij formatters don't end up agreeing with each other, which is one of the big reasons we invested in palantir-java-format.

I assume you work on the 'large internal project' which doesn't yet have pjf applied? In some ways maybe it's fine to just tinker with the eclipse.xml, just wanted to flag that it's always going to be suboptimal until that project gets onto pjf.

@hesselink
Copy link
Contributor Author

Yeah, this is for 'large internal project', where we use the Eclipse formatter plugin for IntelliJ. I tested this change on that project and it doesn't reformat any existing code (it does when the alignment is set to 0).

Copy link
Contributor

@iamdanfox iamdanfox left a comment

Choose a reason for hiding this comment

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

I haven't installed Eclipse to try this out locally, but I don't want to block it given that you are one of the very few eclipse users so if it's broken I trust you'll fix it :)

@carterkozak
Copy link
Contributor

Let's try running this on our large internal project before we merge and release, that way we don't get surprised by unexpected changes.
There are a lot of options that result in broken NLS markers.

@hesselink
Copy link
Contributor Author

@carterkozak As I mentioned above, I did run it on that project (`./gradlew spotlessApply), there were no changes (I actually tweaked the alignment from 0 to 12 to make that happen. It's quite finicky, at 16 it no longer works (and created the old formatting).

@carterkozak
Copy link
Contributor

👍

@bulldozer-bot bulldozer-bot bot merged commit c4fd4b2 into develop Jun 16, 2020
@bulldozer-bot bulldozer-bot bot deleted the dont-wrap-method-declaration branch June 16, 2020 18:42
@svc-autorelease
Copy link
Collaborator

Released 3.27.0

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

Successfully merging this pull request may close these issues.

4 participants