grpc / grpc-java

The Java gRPC implementation. HTTP/2 based RPC
https://grpc.io/docs/languages/java/
Apache License 2.0
11.37k stars 3.82k forks source link

Solve formatting problems once and for all #1664

Closed carl-mastrangelo closed 3 months ago

carl-mastrangelo commented 8 years ago

...by submitting to our benevolent autonomous overlords. The Google standard formatter would solve all formatting debates, and could be pulled in as part of our tests:

https://github.com/google/google-java-format

ejona86 commented 8 years ago

We "only" need Gradle integration.

jdcormie commented 3 months ago

Manually formatting code all the time is driving me crazy. Would any of the 3p gradle integrations listed here be acceptable ?

ejona86 commented 3 months ago

I'm not aware of any reasonable Java code formatter. By reasonable I mean, "does not reformat properly formatted lines." All the ones I'm aware of for Java first throw away existing whitespace. That's not okay, and means everyone must be using the same exact version of the formatter.

Everyone saw the success of gofmt but the thing that made it revolutionary is it only reformatted bad lines.

I've given up on the google java formatter working for us. Inside google it is integrated to only reformat changed lines, which is the minimum we'd need since full-file reformatting that throws away existing formatting is a non-starter.

I don't think we're going to get such tooling any time soon, so I'm going to close this. If there's an IDE option that works for someone, they can commit a configuration file (not a full project file) if that's possible.

jdcormie commented 3 months ago

Thanks for explaining!

Clearly you've thought about this more than I have but it looks like both of these plugins let you specify the exact version of google-java-format to pull in as a build tool. So wouldn't everyone be automatically using the same exact version of the formatter just by typing 'gradle googleJavaFormat'? Maybe this gets upgraded from time to time, but accompanied by a single PR that globally applies any formatting changes that come with the new version?

ejona86 commented 3 months ago

Personally I have zero interest in the Google formatter because it throws away existing whitespace. That's a deal breaker for a language like Java (I have little issue with Buildifier or other data-formatting). If people could use it for just the lines they changed, I'd be fine with them using it. I'm fine with making some concessions in the style for the formatter, but saying "it must be exactly what this specific version of a formatter happens to use" is not something I want to take part in. Even inside Google it is not recommended (last I checked) to wholesale reformat files with the Google formatter.

I think for myself, most correctable checkstyle failures are cases where 1) it is wrong (but I accept) or 2) imports need fixing. There's definitely cases where a tool can't fix it, like missing javadoc. A tool just fixing imports would be much easier, as there is only one way to encode the imports.

jdcormie commented 3 months ago

OK. Looks like gjf does have a script that can restrict itself to just the lines of a change: https://github.com/google/google-java-format/blob/master/scripts/google-java-format-diff.py I'll give that a try.

jdcormie commented 3 months ago

Found the internal guidance for wholesale reformatting: go/java-style#s8.5.1-reformatting-existing-code. It's actually not discouraged, instead the idea is "You change it, you own it". Moreover, "When adding new code to a file that is not in Google Style, reformatting the existing code first is recommended" (8.5.2)

ejona86 commented 3 months ago

From go/google-java-format: "We don't recommend reformatting entire files as part of your regular development workflow." With some language about using incremental. Part of it is about mixing formatting and non-formatting changes together.

jdcormie commented 3 months ago

Right - They're saying don't reformat the entire file that contains a non-formatting change just because it's as easy as pressing Ctrl-Alt-L and "I'm already in there". They close with "You can still periodically submit formatting-only changes to bring entire files into shape" which is what I was suggesting above if we ever adopted gjf as a build tool with version fixed by centralized gradle config. Both initially and in case we upgrade gjf. Found https://github.com/robolectric/robolectric/issues/9163 where they appear to be taking this approach.

Formatting just the lines that changed would be fine with me too - I just want a "fix everything" script I can run before pushing so I never have to think about formatting or hear about it from a reviewer. google-java-format-diff.py appears to do that.

ejona86 commented 3 months ago

CC @larry-safran