Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

davido
Copy link

@davido davido commented May 28, 2017

Fixes: #105.

Test Plan:

$ git show --name-only HEAD | grep java$ | xargs -r gjf --dry-run
Need Formatting:
[gerrit-common/src/main/java/com/google/gerrit/common/IoUtil.java,
gerrit-server/src/main/java/com/google/gerrit/server/account/Emails.java]

@jbduncan
Copy link
Contributor

@davido I believe this is a duplicate of #106.

@davido
Copy link
Author

davido commented May 28, 2017

@jbduncan Oh, indeed, I missed it. And thanks for letting me know.

@jbduncan
Copy link
Contributor

You're very welcome! :)

@sormuras
Copy link
Contributor

sormuras commented May 28, 2017

And still waiting for an reply from the Google guys. The jitpack solution is a good work-around, though. :)

@dpursehouse
Copy link

@cushon any chance to get this or #106 merged? Thanks!

@davido
Copy link
Author

davido commented Jul 1, 2017

Interestingly, the buildifier tool, that is another Google's open source tool for formatting of Blaze and Bazel files, offers the --mode=check|diff|fix option: [1]. Unfortunately, the return code doesn't reflect the outcome of the check: [2].

@@ -32,6 +32,8 @@
"Usage: google-java-format [options] file(s)",
"",
"Options:",
" -n, --dry-run",
" Don't change anything, only check; supposed to be used form the CI.",
Copy link
Author

Choose a reason for hiding this comment

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

supposed to be used from the CI

@davido
Copy link
Author

davido commented Jul 9, 2017

Actually, different check modes are needed: fix|check|diff. This is what Googel's buildifier is offering, so that a CI could run -mode=check first, to see if there were any format issues, and re-run it again with diff mode, to be more verbose on what is actually wrong. That way, tools, that support online editing, like Gerrit Code Review, could address the offending format issues and re-upload new patch set for verification. And all this in your favorite browser.

Example for this approach could be seen in Tensorflow's ci_sanity.sh script.

https://github.com/tensorflow/tensorflow/blob/ce717be4a2966a268e9a4831c39254b611f1c9e7/tensorflow/tools/ci_build/ci_sanity.sh#L264-L294

Fixes: google#105.

Test Plan:

  $ git show --name-only HEAD | grep java$ | xargs -r gjf --dry-run
  Need Formatting:
    [gerrit-common/src/main/java/com/google/gerrit/common/IoUtil.java,\
     gerrit-server/src/main/java/com/google/gerrit/server/account/Emails.java]
@cushon
Copy link
Collaborator

cushon commented Oct 12, 2017

#106 has been merged.

@cushon cushon closed this Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants