Add dry-run mode and --set-exit-if-changed option #106

sormuras · 2016-12-14T15:17:50Z

--dry-run or -n
Prints the paths of the files whose contents would change if the formatter were run normally.

--set-exit-if-changed
Return exit code 1 if there are any formatting changes.

jbduncan

I saw a few areas for improvement in this PR, so I decided to note my thoughts down in the hope that it will make this PR more likely to meet Google's high level of standards. :)

jbduncan · 2016-12-14T15:21:28Z

core/src/main/java/com/google/googlejavaformat/java/CommandLineOptions.java

-          this.removeJavadocOnlyImports);
+          this.removeJavadocOnlyImports,
+          this.validationMode
+      );


I think this bracket should be on the previous line.

jbduncan · 2016-12-14T15:22:30Z

core/src/test/java/com/google/googlejavaformat/java/MainTest.java

+            System.in);
+    int errorCodeOk = main.format("--validate", //
+        tempJava("Simple", "enum Simple {}\n")//
+    );


jbduncan · 2016-12-14T15:22:36Z

core/src/test/java/com/google/googlejavaformat/java/MainTest.java

+    int errorCode = main.format("--validate", //
+        tempJava("IsFormatted", "interface IsFormatted {}\n"), //
+        tempJava("NeedsFormatting", "  class  NeedsFormatting  {  }\n") //
+    );


jbduncan · 2016-12-14T15:24:54Z

core/src/test/java/com/google/googlejavaformat/java/MainTest.java

+  }
+
+  private String tempJava(String name, String code) throws Exception  {
+    Path path = testFolder.newFile( name + ".java").toPath();


There's an unneeded space between the bracket and name.

May I suggest running google-java-format on the files touched by this PR to make sure everything's nice and compliant? :)

jbduncan · 2016-12-14T15:28:52Z

core/src/test/java/com/google/googlejavaformat/java/MainTest.java

+            new PrintWriter(System.out, true),
+            new PrintWriter(System.err, true),
+            System.in);
+    int errorCodeOk = main.format("--validate", //


Oh, it's also not clear to me why you included blank comments at the end of this line and a few of the lines below. Could you explain why they're there? Are they even needed?

No, not needed at all. And gone.

Great, current version looks good to me now. :)

sormuras · 2016-12-14T18:53:02Z

core/src/main/java/com/google/googlejavaformat/java/Main.java

+          continue; // no formatting needed, validate next
+        }
+        errWriter.print(result.getKey() + ": needs formatting");
+        return 1; // found first file that needs formatting - exit with error code 1


Could use allOk flag w/ continue to print all files failing format validation here.

hanwen · 2017-03-21T12:31:57Z

core/src/main/java/com/google/googlejavaformat/java/Main.java

+          continue; // no formatting needed, validate next
+        }
+        errWriter.print(result.getKey() + ": needs formatting");
+        return 1; // found first file that needs formatting - exit with error code 1


can this print all files that need formatting? It's much more useful to get a list of all files that need formatting.

Aye. See comment above, at #106 (review)

sormuras · 2017-04-27T20:01:15Z

@ronshapiro @cushon Any chance for the validation mode to get integrated?

f2prateek · 2017-04-27T20:07:48Z

This will be great to have! One thing I'll point out is that in practice I've found it extremely helpful to explain what is expected and what it received — here's how I implemented that in the Gradle Android plugin f2prateek/gradle-android-javafmt-plugin#6 (comment)

sormuras · 2017-05-04T18:24:47Z

Seeing a diff is helpful in general -- but when it comes to google-java-format you just let it do its work. After a normal run, a failing validation check points to a bug in the formatter. See FormatterIntegrationTests#idempotent... methods.

sormuras · 2017-05-04T18:26:54Z

@cushon One more ping from my side regarding the "validation" feature. Do you think it's worth to have it? I can live with a simple no ... and start using my fork of the project. :)

f2prateek · 2017-05-04T18:27:30Z

That's fair, I do point out in the failure about running fmt, but I also wanted to make it easy for contributors to see what exactly was different.

sormuras · 2017-05-24T18:56:40Z

Rebased.

sormuras · 2017-06-07T18:11:23Z

Rebased.

sormuras · 2017-08-05T09:16:00Z

Rebased.

sormuras · 2017-08-11T16:37:22Z

Rebased.

@cushon @plumpy Any news on having a validation / dry-run mode in the CLI of GJF?

sormuras · 2017-08-22T19:22:37Z

Rebased.

Using https://jitpack.io/com/github/sormuras/google-java-format/validate-SNAPSHOT/ from my fork is an option -- but any answer, be it yes or no, would release me from rebasing that branch. 😉

sormuras · 2017-08-29T19:03:05Z

Rebased to 1.4

sormuras · 2017-10-09T18:13:54Z

Yes.
No.
Next year.

😉

cushon · 2017-10-09T19:08:43Z

Sorry for neglecting this. I asked a clarifying question in #105.

sormuras · 2017-10-09T20:07:12Z

Rebased to 1.5-SNAPSHOT and really like the activity here and in #105 😎

cushon · 2017-10-11T00:42:31Z

core/src/main/java/com/google/googlejavaformat/java/Main.java

+        if (idempotent) {
+          continue; // no formatting needed, validate next
+        }
+        errWriter.println(path + ": needs formatting");


I'd expect stdout for this, and dropping : needs formatting would make this easier to use in scripts.

cushon · 2017-10-11T00:43:45Z

core/src/main/java/com/google/googlejavaformat/java/Main.java

          e.getCause().printStackTrace(errWriter);
        }
        allOk = false;
        continue;
      }
+      boolean idempotent = formatted.equals(inputs.get(path)); // not changed


perhaps more self-documenting:

boolean changed = !formatted.equals(inputs.get(path));

cushon · 2017-10-11T00:44:26Z

core/src/main/java/com/google/googlejavaformat/java/Main.java

-        outWriter.write(formatted);
+        if (parameters.isSetExitIfChanged()) {
+          allOk = false;
+        }


Can you move this if to immediately below the declaration of idempotent/changed to avoid some duplication?

Not so easy. The default "stdout" is implemented as a fall-through or "catch-all-else". Will try.

Sorry, what am I missing?

index 7b7c873..b331a78 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/Main.java +++ b/core/src/main/java/com/google/googlejavaformat/java/Main.java @@ -153,14 +153,14 @@ public final class Main { continue; } boolean idempotent = formatted.equals(inputs.get(path)); // not changed + if (!idempotent && parameters.isSetExitIfChanged()) { + allOk = false; + } if (parameters.isValidationMode()) { if (idempotent) { continue; // no formatting needed, validate next } errWriter.println(path + ": needs formatting"); - if (parameters.isSetExitIfChanged()) { - allOk = false; - } continue; } if (parameters.inPlace()) { @@ -174,15 +174,9 @@ public final class Main { allOk = false; continue; } - if (parameters.isSetExitIfChanged()) { - allOk = false; - } continue; } outWriter.write(formatted); - if (parameters.isSetExitIfChanged() && !idempotent) { - allOk = false; - } } return allOk ? 0 : 1; }

I'll push a cleaner solution. It needs logical "is-not-running-in-special-mode" helper in CommandLineOptions:

/** Returns true if no special run-mode (like in-place or dry-run) is selected. */ boolean stdout() { return !(inPlace || dryRun); }

Btw. should we check for "in-place" and "dry-run" are never both set at the same time?

Making the combination a usage error sounds fine.

I'm still not sure I understand what the helper is needed for?

Maybe I'm assuming s/t wrong here.

Should the stdout-case always print out the potentially formatted text?

google-java-format T.java always prints formatted text to stdout, regardless of whether the input was formatted. google-java-format -n T.java prints T.java to stdout if the input was not already formatted.

The current iteration behaves like this. I don't see an easy way to re-use a boolean state at the moment.

cushon · 2017-10-11T00:46:25Z

core/src/main/java/com/google/googlejavaformat/java/UsageException.java

+    "  --validate, -validate",
+    "    Validate files format only. If files need format print its path to stdout.",
+    "  --set-exit-if-changed, -set-exit-if-changed",
+    "    Validate files only, do not write anything. Error code 0 means everything's formatted.",


I prefer:

--dry-run -n Prints the paths of the files whose contents would change if the formatter were run normally. --set-exit-if-changed Return exit code 1 if there are any formatting changes

validation isn't necessarily the only use of these flags.

Support -e as a short-cut for --set-exit-if-changed?

I think it's fine to skip the short form for this one.

cushon · 2017-10-11T00:49:58Z

core/src/main/java/com/google/googlejavaformat/java/Main.java

+      }
+      outWriter.write(formatted);
+      if (parameters.isSetExitIfChanged() && !idempotent) {
+        allOk = false;
      }
    }
    return allOk ? 0 : 1;


I expected to see changes to formatStdin too?

Not sure about this one.

Should an exit code of 1 be returned if changes where made for input from stdin and --set-exit-if-changed is set to true?

You are referring to the method private int formatStdin(CommandLineOptions parameters, JavaFormatterOptions options) {...}!

I would expect -n and --set-exit-if-changed to work the same for input from stdin as it does for files. For google-java-format -n - we can reuse STDIN_FILENAME.

Done via:

if (parameters.isDryRun()) { outWriter.println(STDIN_FILENAME); } else { outWriter.write(output); } if (input.equals(output)) { return 0; } return parameters.isSetExitIfChanged() ? 1 : 0;

sormuras · 2017-10-11T07:02:54Z

core/src/main/java/com/google/googlejavaformat/java/Main.java

+      }
+      boolean changed = !formatted.equals(inputs.get(path));
+      if (!changed) {
+        continue; // preserve original file


Could be transformed to

if (changed) { ... }

for clarity. Renders the continue obsolete.

`--dry-run` or `-n` Prints the paths of the files whose contents would change if the formatter were run normally. `--set-exit-if-changed` Return exit code 1 if there are any formatting changes.

sormuras · 2017-10-11T07:41:43Z

I'll leave the PR now as-is. If anything needs a tweak or correction, let me know.

Cheers!

cushon · 2017-10-11T07:44:21Z

core/src/main/java/com/google/googlejavaformat/java/Main.java

-      outWriter.write(output);
-      return 0;
+      if (parameters.isDryRun()) {
+        outWriter.println(STDIN_FILENAME);


Let's only do this if the input was unformatted, same as for file inputs.

sormuras · 2017-10-11T11:31:43Z

Cleaned up and tests added.

`--dry-run` or `-n` Prints the paths of the files whose contents would change if the formatter were run normally. `--set-exit-if-changed` Return exit code 1 if there are any formatting changes. See #105, #106 MOE_MIGRATED_REVID=171899496

cushon · 2017-10-12T02:35:25Z

Merged as 13354f3.

I made some adjustments but nothing that should have changed behaviour. Let me know if I missed anything, and thanks for your work on this!

sormuras · 2017-10-12T06:26:36Z

Build via https://jitpack.io/com/github/google/google-java-format/master-SNAPSHOT/build.log
Download all-deps from https://jitpack.io/com/github/google/google-java-format/google-java-format/master-SNAPSHOT/google-java-format-master-SNAPSHOT-all-deps.jar

java -jar google-java-format-master-SNAPSHOT-all-deps.jar --version

yields:

google-java-format: Version 1.5-SNAPSHOT

And running java -jar google-java-format-master-SNAPSHOT-all-deps.jar --dry-run --set-exit-if-changed on a tree of un-formatted sources exits with error code 1 as expected.

✔️

Cheers! See google/google-java-format#106

sormuras · 2017-10-12T06:44:59Z

Updating my project to consume GJF 1.5-SNAPSHOT instead of my fork was trivial: sormuras/bach@99d48cb

Summary: In --dry-run mode, ktfmt will print the file paths of files that need formatting but will not change the file contents. Also added the option to exit the program with code `1` if any changes are needed rather than just if any errors happened. These changes are in alignment with google-java-format's identical options (see google/google-java-format#106). Usage: --dry-run or -n Prints the paths of the files whose contents would change if the formatter were run normally. --set-exit-if-changed Return exit code 1 if there are any formatting changes made (or detected if running in dry mode). Reviewed By: cgrushko Differential Revision: D31286652 fbshipit-source-id: c89fb72a1411766b81c28eba66614df78b0fea0a

googlebot added the cla: yes label Dec 14, 2016

jbduncan suggested changes Dec 14, 2016

View reviewed changes

jbduncan reviewed Dec 14, 2016

View reviewed changes

sormuras commented Dec 14, 2016

View reviewed changes

sormuras mentioned this pull request Feb 9, 2017

Prevent edit if nothing was formatted #120

Merged

sormuras mentioned this pull request Feb 24, 2017

The plugin adds a google-java-format formatter implementation that can be configured in Window > Preferences > Java > Code Style > Formatter > Formatter Implementation #134

Closed

olim7t mentioned this pull request Mar 1, 2017

Validation mode spotify/fmt-maven-plugin#10

Merged

sormuras mentioned this pull request Mar 21, 2017

allow running gjf as test #140

Closed

hanwen reviewed Mar 21, 2017

View reviewed changes

sormuras mentioned this pull request Apr 4, 2017

Plugin didn't change wrong name of var #142

Closed

sormuras mentioned this pull request May 24, 2017

Implement validation mode for Bach#format() sormuras/bach#6

Closed

jbduncan mentioned this pull request May 28, 2017

Add --dry-run option to simplify CI system integrations #154

Closed

jbduncan mentioned this pull request Jun 28, 2017

Format code base jrtom/jung#86

Closed

sormuras mentioned this pull request Aug 22, 2017

JDK 9 module-info.java handling #75

Closed

cushon self-assigned this Oct 9, 2017

sormuras mentioned this pull request Oct 10, 2017

Validation mode to check if file is correctly formatted #105

Closed

cushon requested changes Oct 11, 2017

View reviewed changes

sormuras commented Oct 11, 2017

View reviewed changes

sormuras changed the title ~~Add validation mode.~~ Add dry-run mode and --set-exit-if-changed option Oct 11, 2017

Add --dry-run and --set-exit-if-changed option to CLI

ee18048

`--dry-run` or `-n` Prints the paths of the files whose contents would change if the formatter were run normally. `--set-exit-if-changed` Return exit code 1 if there are any formatting changes.

cushon reviewed Oct 11, 2017

View reviewed changes

Cleanup

b14d8da

cushon closed this Oct 12, 2017

sormuras deleted the validate branch October 12, 2017 05:28

sormuras added a commit to sormuras/bach that referenced this pull request Oct 12, 2017

Use '--dry-run' and switch from validate-fork to master-SNAPSHOT

99d48cb

Cheers! See google/google-java-format#106

davido mentioned this pull request Oct 15, 2017

Release new gjf version #214

Closed

jbduncan mentioned this pull request Aug 1, 2018

Remove unused import google/startup-os#215

Merged

Kernald mentioned this pull request Apr 21, 2021

Validate/dry-run mode facebook/ktfmt#212

Closed

Add dry-run mode and --set-exit-if-changed option #106

Add dry-run mode and --set-exit-if-changed option #106

Conversation

sormuras commented Dec 14, 2016 • edited Loading

jbduncan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sormuras commented Apr 27, 2017

f2prateek commented Apr 27, 2017

sormuras commented May 4, 2017

sormuras commented May 4, 2017

f2prateek commented May 4, 2017

sormuras commented May 24, 2017

sormuras commented Jun 7, 2017

sormuras commented Aug 5, 2017

sormuras commented Aug 11, 2017

sormuras commented Aug 22, 2017

sormuras commented Aug 29, 2017

sormuras commented Oct 9, 2017

cushon commented Oct 9, 2017

sormuras commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cushon Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sormuras commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sormuras commented Oct 11, 2017

cushon commented Oct 12, 2017

sormuras commented Oct 12, 2017

sormuras commented Oct 12, 2017

sormuras commented Dec 14, 2016 •

edited

Loading

cushon Oct 11, 2017 •

edited

Loading