Validation mode to check if file is correctly formatted

[JLLeitschuh]

This would be useful in CI systems where you would want the build to fail if the user hasn't formatted their code before the CI system runs.

[jbduncan]

FWIW, Spotless (which supports google-java-format) already allows the user to do this. :)

[JLLeitschuh]

We are using maven in our build at the moment (trying to move to gradle but transition is slow).
Anything we can do in our world?
I was thinking about doing a checksum diff as a simple fix unless there is a better solution out there.

[Stephan202]

We have a format.sh script that wraps the Maven plugin, and a Dangerfile containing the following (this is executed by Travis CI, only for PR builds):

# Run the formatter and check whether this introduces any changes. 
if !system('./format.sh && git diff --exit-code --name-status')
  fail 'There are formatting errors, please run `./format.sh`.' 
end

[jbduncan]

@JLLeitschuh Interesting you mention Maven, as we (@nedtwigg and I) are in the middle of writing version 3.0 of Spotless, and one of the things @nedtwigg has been working on recently is splitting out the "core" of Spotless into its own lib so that it can eventually be used in non-Gradle contexts.

However, I don't know if he has any definite plans to support a Maven plugin, so if you're interested, I'd suggest you ask him on the issue he's made to track the spotless-lib efforts. (diffplug/spotless#56)

[JLLeitschuh]

I honestly don't plan on supporting our maven build for that much longer either.
I'm in the process of rewriting our build using gradle groovy and gradle script kotlin however being a startup with many other things going on simeltaneously I've got a lot of moving targets.

You've been a lot of help.
Do you mind sharing the contents of your ./format.sh @Stephan202 ? Is it just the call out to the google-java-format jar??

[Stephan202]

@JLLeitschuh: there's some other (actually unused) stuff in there, but the no-arg invocation boils down to:

mvn -B -T 1.0C com.github.PicnicSupermarket:googleformatter-maven-plugin:format

Where the referenced plugin is a Jitpack'ed version of this branch, forked off talios/googleformatter-maven-plugin. (I recommend using the latest version instead. Reason I haven't upgraded our script yet is that I need to make sure we have precisely matching versions of the IDEA and Eclipse plugins. For that I am waiting on #80.)

[mernst]

The run-google-java-format project has a check-google-java-format.py script that checks whether a file is properly formatted. The project's README file tells how to integrate with make, ant, gradle, git pre-commit hooks, and gives some advice about running google-java-format for the first time.

[sormuras]

I still think, a validation mode is a useful core addition.

[alexkleiman]

I agree that this would be very useful as a core feature. I imagine that nearly everybody using google-java-format within a large organization maintains some sort of CI system to enforce that code is formatted properly. In any organization with more than a handful of developers, it is essential to ensure that every developer always submits code formatted to google-java-format's standards. Otherwise, there is no way to maintain a codebase's formatting without constantly causing merge conflicts for developers whenever a reformatting event takes place.

For these reasons, I view a validation mode as a hard requirement for use of google-java-format in a large organization. Because this is a requirement for use of google-java-format at scale, it makes sense as a core feature.

[sormuras]

FWIW, you may use https://jitpack.io to get a compiled version of google-java-format with integrated validation mode:

https://jitpack.io/#sormuras/google-java-format/validate-SNAPSHOT

If you want to just download the all-deps JAR, you can grab it here: https://jitpack.io/com/github/sormuras/google-java-format/google-java-format/validate-SNAPSHOT/google-java-format-validate-SNAPSHOT-all-deps.jar

I'll try to keep the validate branch as up-to-date as possible.

[cushon]

Are there advantages to adding this to the CLI over using something like the google-java-format <file> | diff <file> - to check if a file is already formatted?

For batch processing multiple files a single invocation of google-java-format would be faster, but we recommend validating only changed lines instead of the entire fire, which is only possible one file at a time anyways. The formatting algorithm will continue to change, so trying to be 100% compliant with the latest version is going to result in churn from unrelated formatting changes when editing files.

[sormuras]

The advantage for me is that I often don't have a) versioning system in place for a tree of sources and b) operate on a system where piping and diffing is not available "by default".

A change in the formatting algorithm is a non-issue. I'm content to be locked-in to gjf w/o configuration support. And a project does not need to upgrade to the latest formatter version, if this is a problem.

[alexkleiman]

@cushon yes, this would be useful.

For batch processing multiple files a single invocation of google-java-format would be faster, but we recommend validating only changed lines instead of the entire fire, which is only possible one file at a time anyways.

I believe a common setup is:

1. CI treats any code which doesn't correspond to google-java-format standards as a build failure
2. Thanks to (1), running the formatter against an entire repo is equivalent to validating only changed lines in a file (all non-changed lines are already formatted).
3. If a new version of google-java-format introduces formatting changes, the entire codebase is reformatted as part of the upgrade to this new version of google-java-format. This allows (1) to continue.

A validation mode would make implementing (1) significantly easier.

The formatting algorithm will continue to change, so trying to be 100% compliant with the latest version is going to result in churn from unrelated formatting changes when editing files.

See (3). By reformatting the entire codebase when upgrading versions of google-java-format, this issue is avoided. Many users of google-java-format are content to deal with this upgrade process.

Furthermore, the community desire for this feature is illustrated by the numerous open-source project which have added an unofficial validation mode, either by forking google-java-format or writing a build tool which compares the text of a file before and after applying the formatter:
https://github.com/plume-lib/run-google-java-format
https://github.com/PicnicSupermarket/googleformatter-maven-plugin/tree/bleeding-edge-google-java-format
https://github.com/sormuras/google-java-format
https://github.com/talios/googleformatter-maven-plugin
https://github.com/sherter/google-java-format-gradle-plugin

I would ask that you once again consider implementing this much-desired feature.

Thanks!

[jbduncan]

Furthermore, the community desire for this feature is illustrated by the numerous open-source project which have added an unofficial validation mode, either by forking google-java-format or writing a build tool which compares the text of a file before and after applying the formatter: ...

Not to mention that there's also https://github.com/diffplug/spotless which does the same thing as the other projects that @alexkleiman listed.

[jbduncan]

...which BTW is still awaiting review to be mentioned in the README. 😉

[cushon]

There's some precedent: gofmt -l and dartfmt -n both print names of unformatted files. The author of clang-format recommends clang-format <file> | diff <file> -.

@sormuras how does this help with the lack of a versioning system?

@alexkleiman are you interested in an addition to the API here, or just the CLI? The examples that use the API look fine: https://github.com/sherter/google-java-format-gradle-plugin/blob/3666fb45e5ff0b85b6fb2cd5b262c92f78dc60d8/src/main/java/com/github/sherter/googlejavaformatgradleplugin/FormatFileCallable.java#L44

[cushon]

Also, is there consensus that a dry run mode should exist with a non-zero status if unformatted files were found? That's a departure from gofmt.

[sormuras]

Also, is there consensus that a dry run mode should exist with a non-zero status if unformatted files were found? That's a departure from gofmt.

That's why I like "validate" / "validation mode" phrase. When a validation run returns zero, everything's fine. Indicating zero errors occured. A non-zero status means format validation failed.

@sormuras how does this help with the lack of a versioning system?

Mh, no versioning system, no diff support, no easy way to tell if files needs formatting.

[jbduncan]

@cushon I would like to add that, for those of us who use Windows or AppVeyor, diff programs are not installed by default, and thus for me at least, it's far from obvious (1) what open-source command-line diff programs are available, and (2) how to chain the results of google-java-format with such a diff program to get the equivalent of a CI-friendly validation mode.

Thus, if precedence from gofmt and dartfmt dictates that google-java-format should not have a built-in validation mode, then I'd strongly advocate for there to be some sort of instructions in the README on how to use google-java-format as a validation mechanism in popular CI systems like Travis CI, AppVeyor and Jenkins.

[cushon]

if precedence from gofmt and dartfmt dictates that google-java-format should not have a built-in validation mode

Sorry, I was trying to say the opposite of that. gofmt -l and dartfmt -n both print the paths of the files whose contents would change if the formatter were run normally, which I think is what's being requested here. (clang-format is the exception, but that's ok.)

That's why I like "validate" / "validation mode" phrase. When a validation run returns zero, everything's fine. Indicating zero errors occured. A non-zero status means format validation failed.

Is printing the paths of files that need to be formatted also sufficient for that? Neither dartfmt or gofmt set the exit status in dry run mode by default, but dartfmt has an option (--set-exit-if-changed) to enable that behaviour.

[alexkleiman]

@cushon thanks for your response!

tl;dr - If I had to choose, I'd easily go with the CLI, but an API addition would be a nice-to-have.

are you interested in an addition to the API here, or just the CLI?

Ah, I'm sorry for not specifying that earlier. I'm more interested in the CLI command, but would be interested in both, for different reasons.

The CLI command would be great for all the reasons listed above. With just a line or two of code, it would be possible to write the desired automation which uses google-java-format.

An API addition could be useful as an optimization. An API method would:

• Eliminate the need to read the formatted and unformatted versions of a file into memory
• Eliminate the need to perform string comparisons on these two versions
• Potentially eliminate the need to format an entire file, as a file with a formatted error on line 1 doesn't need to be examined any further.

Now, this would only be a benefit if the API call didn't do all of this under the hood. I wouldn't expect the API call to be more efficient on it's first implementation, but one can always dream for the future, right?

One additional consideration: it's easier for Gradle plugins to lead the google-java-format library on the runtime classpath and call its API than it is to make a CLI call. Using the API also enables users to easily specify the version of google-java-format they want the Gradle plugin to use, as is done with the gradle-errorprone-plugin.

Thanks for your thoughtful consideration of this feature request!

[sormuras]

That's why I like "validate" / "validation mode" phrase. When a validation run returns zero, everything's fine. Indicating zero errors occured. A non-zero status means format validation failed.

Is printing the paths of files that need to be formatted also sufficient for that? Neither dartfmt or gofmt set the exit status in dry run mode by default, but dartfmt has an option (--set-exit-if-changed) to enable that behaviour.

Only printing the paths of files that need to be formatted would yield a "output analyzing" post-processing pass to check if s/t was printed or not.

The original request here reads:

This would be useful in CI systems where you would want the build to fail if the user hasn't formatted their code before the CI system runs.

Having a non-zero exit code in "validation mode" could reach that goal w/o any extra effort.

You don't like the non-zero exit code. Why?

This how I use the modified GJF at the moment:

java -jar google-java-format-validate-SNAPSHOT-all-deps.jar --validate [files...]

I could live with a second option to enable the "--set-exit-to-non-zero-if-validation-fails" behaviour. The usage would be enhanced to:

java -jar google-java-format-1.5-SNAPSHOT-all-deps.jar --validate --set-exit-to-non-zero-if-validation-fails [files...]

[cushon]

You don't like the non-zero exit code. Why?

I just pointed out that other formatters don't do that by default. Making the interfaces similar makes it easier to integrate them into other tools (e.g. for presubmit checks...), and makes it easier to learn them. It sounds like the way dartfmt handles this would work for your use-case?

[sormuras]

Redirect the output, if any, to a file and test for its existence using OS dependent utilities. Feels not so good, but manageable.

Following dartfmt -n-style, I'll guard the allOk = false line with an configurable switch. Plus an update to the command line description and the test case?

[sormuras]

... but dartfmt has an option (--set-exit-if-changed) to enable that behaviour.

Is this switch limited to the dry run mode? Or if set to true does dartfmt always report an error code if it changed (printed the file paths in dry run) some files?

[sormuras]

See 00b6b53 in #106 for --set-exit-if-changed option implementation. The exit code is only set to 1 in inPlace or validate mode.

What about the stdout case?

[sormuras]

If I understand https://github.com/dart-lang/dart_style/blob/master/lib/src/formatter_options.dart#L216 correctly, it always returns 1 if any changes are made. That includes the stdout case.

I'll update the PR according to this.

[cushon]

always returns 1 if any changes are made. That includes the stdout case.

That matches what I'm seeing, and seems reasonable.

Do you have concerns about printing the names of files that need formatting?

[sormuras]

No. See https://github.com/google/google-java-format/pull/106/files#diff-e2a1fb24af1354460a3ee75611e8a298R160

A small cleanup is being forced pushed in a minute...

[sormuras]

...pushed.

[sormuras]

After merging #106 this issue is finally closeable. 🌞