Skip to content

Format Only Changes #197

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Format Only Changes #197

wants to merge 4 commits into from

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Feb 17, 2025

  • sorts Context/Jex members
  • adds auto-formatter
  • format code with the google java style

@SentryMan SentryMan changed the title Format Changes Format Only Changes Feb 17, 2025
@SentryMan SentryMan enabled auto-merge (squash) February 17, 2025 18:40
@rbygrave
Copy link
Contributor

format code with the google java style

I'm unconvinced by this.

@SentryMan
Copy link
Collaborator Author

I'm unconvinced by this.

Is there any particular reason?

Pros:

  • It all but eliminates "diff noise" due to whitespace and other insignificant formatting choices, allowing contributors/reviewers to focus on logic and functionality.
  • Enforces code consistency no matter what tool you use to write code
  • By automating formatting, contributors are less likely to introduce manual formatting mistakes.

Cons:

  • Removes the ability to do any 'special-case' formatting where an alternate format would make code more readable in some cases.

@rbygrave
Copy link
Contributor

Is there any particular reason?

For me, it fails aesthetically in too many common cases with too much indentation (double and quadrupedal indentation) and line breaking.

There are formatters that don't aesthetically fail as much as google java style / basically closer to IntelliJ style.

@SentryMan
Copy link
Collaborator Author

do you have one in mind that I can setup automatic formatting with?

@SentryMan
Copy link
Collaborator Author

SentryMan commented Feb 17, 2025

For me I couldn't care less about any particular code style, it's just that a standard format that can be enforced automatically makes contribution/reviewing easier.

@SentryMan
Copy link
Collaborator Author

what do you think about https://github.com/palantir/palantir-java-format

@rbygrave
Copy link
Contributor

what do you think about https://github.com/palantir/palantir-java-format

I have been trying it out actually, but yes it does kinda suck in some DSL cases [e.g. ebean query bean queries] so I'm not 100% sold on it. Still looking.

@SentryMan
Copy link
Collaborator Author

it does kinda suck in some DSL cases [e.g. ebean query bean queries]

okay, but are those DSL cases in this repo/pr? I'm not talking about changing what you use for application development.

@SentryMan
Copy link
Collaborator Author

are those DSL cases in this repo/pr?

@rbygrave do any of the diffs in this pr have any formatting issues that will prevent you from accepting the auto-formatter on this repo?

@rbygrave
Copy link
Contributor

rbygrave commented Mar 4, 2025

I'm still trialing the IntelliJ plugin and that wasn't actually working as I expected yet [which I realise sounds weird, I thought it would "just work" but not really] ... plus I am wondering about the specifics on tabs/spaces ... plus pondering if formatting should actually fail a PR or autoformat.

... so those are the things that are still WIP in my mind yes, and I'd want to get this "right" rather than do this multiple times because we rushed and got it wrong etc.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Mar 4, 2025

For me I will continue to use the google format in my ide because with this change I'm finally free to not care about formatting.

This pr doesn't auto-format, it fails the PR with a message to use the mvn spotless apply goal.

@SentryMan
Copy link
Collaborator Author

To speak plainly, the whole point of this pr is so that you don't have to setup/change any IDE/plugin settings.

@SentryMan
Copy link
Collaborator Author

Well, it seems palantir isn't gonna fly, (text blocks don't work)

There are formatters that don't aesthetically fail as much as google java style

what are these other formatters? (and can they be used for auto formatting)

@SentryMan
Copy link
Collaborator Author

prettier is not a good option since it requires a node installation and npm install -g prettier-plugin-java

@agentgt
Copy link

agentgt commented Mar 10, 2025

@rbygrave FWIW I use one of the following in my projects

  1. https://github.com/revelc/formatter-maven-plugin
  2. https://github.com/spring-io/spring-javaformat
  3. https://github.com/diffplug/spotless

All of the above are (or can) use Eclipse JDT (headless and only the formatting portion of ECJ).

The first one you just give it an Eclipse formatting properties file. The second one has Springs eclipse formatting properties baked in. The third (Spotless) is like the first one but allows different file types and different formatters.

All of the JStachio projects use spring-javaformat and all of my companies projects use the formatter-maven-plugin.

I mostly do not care about formatting so long as the build goes ahead and fixes it for me however I don't like the google java format. I think indenting by 2 spaces because of line room ... is ... well just break up your code and don't indent that much or maybe not have names that are 40 characters long :).

Intellij has a plugin to read Eclipse format: https://plugins.jetbrains.com/plugin/6546-adapter-for-eclipse-code-formatter

Or you can use the Spotless Intellij plugin (which would then still call Eclipse formatter).

While IntelliJ kicks Eclipse ass on most things it does not have nearly the flexibility last I checked as Eclipse formatter does.

My recommendation is you crack open Eclipse like once and configure the format you want and export it. Then use the above plugins to read that format.

What I do in terms of build is have two Maven profiles. The build machine always uses the profile that will check if the format is correct (usually by diff I think or checksum on multiple runs). Then I have another profile that will automatically reformat. In opensource projects I don't have this profile run unless enabled because people don't like build changing their code but in closed source I have the formatter run on every non-build-machine build. What happens is if someone runs the build it will fail instead and they are then told to rerun the build with some goal that will auto-reformat.

I will say no auto formatting is better than Checkstyle (IMO is garbage and not needed today) but having auto formatting is worth it for consistency.

BTW if the build fails because formatting was not correct ... that means they didn't rebuild before pushing. In a normal PR this will be fine since you wait till green before merge/rebase.

@SentryMan SentryMan force-pushed the format branch 2 times, most recently from 08a8360 to c879113 Compare March 16, 2025 18:40
@SentryMan SentryMan force-pushed the format branch 2 times, most recently from 43ab282 to 3b7953b Compare March 19, 2025 03:46
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