Skip to content
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

Only apply Google Java format to changed lines #176

Merged
merged 6 commits into from Aug 29, 2019

Conversation

@weiminyu
Copy link
Collaborator

commented Jul 15, 2019

The downside is we need to maintain and update a .py file and a jar by ourselves.


This change is Reviewable

@weiminyu weiminyu requested review from jianglai, CydeWeys, gbrodman and hstonec Jul 15, 2019

@googlebot googlebot added the cla: yes label Jul 15, 2019

@gbrodman
Copy link
Collaborator

left a comment

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, and @weiminyu)


java-format/google-java-format_google-java-format-diff.py, line 1 at r1 (raw file):

#!/usr/bin/env python2.7

Where is this file taken from? We probably want to document that, and I'm not sure how this licensing ties in with Apache 2


java-format/google-java-format_google-java-format-diff.py, line 3 at r1 (raw file):

#!/usr/bin/env python2.7
#
#===- google-java-format-diff.py - google-java-format Diff Reformatter -----===#

Is there a reason why "google-java-format" is repeated in the filename?


java-format/google-java-format_google-java-format-diff.py, line 61 at r1 (raw file):

  args = parser.parse_args()

  # Extract changed lines for each file.

Could there be situations where the diffs don't have formatting errors by themselves but might introduce formatting errors in the file? By the documentation, it seems like only the diffs are passed into the formatter.

@weiminyu
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @jianglai)


java-format/google-java-format_google-java-format-diff.py, line 1 at r1 (raw file):

Previously, gbrodman wrote…

Where is this file taken from? We probably want to document that, and I'm not sure how this licensing ties in with Apache 2

Added a README.md file to document origin and licenses. LLVM license grants unrestricted use.


java-format/google-java-format_google-java-format-diff.py, line 3 at r1 (raw file):

Previously, gbrodman wrote…

Is there a reason why "google-java-format" is repeated in the filename?

The original file is named this way.


java-format/google-java-format_google-java-format-diff.py, line 61 at r1 (raw file):

Previously, gbrodman wrote…

Could there be situations where the diffs don't have formatting errors by themselves but might introduce formatting errors in the file? By the documentation, it seems like only the diffs are passed into the formatter.

The file name is also passed to the formatter, which will look around it, and say, taking into existing indentation into consideration.

@CydeWeys
Copy link
Member

left a comment

Out of curiosity, what does it look like when formatting is applied to every file in the project?

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @jianglai)

@weiminyu
Copy link
Collaborator Author

left a comment

weiminyu@92218b9

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @jianglai)

@weiminyu
Copy link
Collaborator Author

left a comment

That's the clickable link to the github branch.

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @gbrodman, @hstonec, and @jianglai)

@gbrodman
Copy link
Collaborator

left a comment

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @hstonec, @jianglai, and @weiminyu)


java-format/google-java-format_google-java-format-diff.py, line 1 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

Added a README.md file to document origin and licenses. LLVM license grants unrestricted use.

Ah, I see the second license in https://github.com/google/google-java-format/blob/master/LICENSE

Odd that they're separated


java-format/google-java-format_google-java-format-diff.py, line 3 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

The original file is named this way.

It doesn't seem that way in https://github.com/google/google-java-format/blob/master/scripts/google-java-format-diff.py


java-format/google-java-format_google-java-format-diff.py, line 61 at r1 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

The file name is also passed to the formatter, which will look around it, and say, taking into existing indentation into consideration.

Interesting -- do you know if it looks at the whole file, just the N lines surrounding the changed text, or maybe the surrounding code block? I wonder how it works exactly.

@jianglai

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Looks like this PR has stalled. Do we still want to do this? I think it is a good practice to run google-java-format on changed files, FWIW.

@gbrodman

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

We still want to do this, right?

@weiminyu
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @gbrodman, @hstonec, @jianglai, and @weiminyu)


java-format/google-java-format_google-java-format-diff.py, line 1 at r1 (raw file):

Previously, gbrodman wrote…

Ah, I see the second license in https://github.com/google/google-java-format/blob/master/LICENSE

Odd that they're separated

The incremental diff script in python is contributed with a different license.


java-format/google-java-format_google-java-format-diff.py, line 61 at r1 (raw file):

Previously, gbrodman wrote…

Interesting -- do you know if it looks at the whole file, just the N lines surrounding the changed text, or maybe the surrounding code block? I wonder how it works exactly.

"it expands each range it is given until it includes a complete region of code that it knows how to format.
This means that it typically reformats entire statements, as well as method and class signatures.
"

@gbrodman
Copy link
Collaborator

left a comment

Can we rename the Python file? Looks good otherwise.

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @hstonec, @jianglai, and @weiminyu)

@jianglai

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

What is happening to this PR...

@weiminyu weiminyu force-pushed the weiminyu:diff-only-format branch from de9e48a to c643d32 Aug 27, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 27, 2019

This pull request introduces 1 alert when merging c643d32 into 844c470 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None
Only apply Google Java format to changed regions
Diffs are relative to origin/master.

Three tasks are added:
- javaIncrementalFormatCheck is added to the build workflow, and
  will abort build if format violations are found.
- javaIncrementalFormatApply needs to be manually invoked to correct
  format violations, the same behavior as spotlessApply.
- javaIncrementalFormatDryRun shows the changes that would happen if
  javaIncrementalFormatApply is invoked.

These tasks work from the root directory and process the buildSrc directory
too.

The Spotless Java config is removed.

@weiminyu weiminyu requested a review from mindhog Aug 28, 2019

@weiminyu
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @gbrodman, @hstonec, @jianglai, and @mindhog)


java-format/google-java-format_google-java-format-diff.py, line 3 at r1 (raw file):

Previously, gbrodman wrote…

It doesn't seem that way in https://github.com/google/google-java-format/blob/master/scripts/google-java-format-diff.py

Done.

Only apply Google Java format to changed regions
Diffs are relative to origin/master.

Three tasks are added:
- javaIncrementalFormatCheck is added to the build workflow, and
  will abort build if format violations are found.
- javaIncrementalFormatApply needs to be manually invoked to correct
  format violations, the same behavior as spotlessApply.
- javaIncrementalFormatDryRun shows the changes that would happen if
  javaIncrementalFormatApply is invoked.

These tasks work from the root directory and process the buildSrc directory
too.

The Spotless Java config is removed.

@weiminyu weiminyu requested review from gbrodman and CydeWeys Aug 28, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 28, 2019

This pull request introduces 1 alert when merging fdcc9ab into 844c470 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None
@mindhog
Copy link
Member

left a comment

Reviewed 1 of 6 files at r1, 1 of 5 files at r2, 3 of 4 files at r3.
Reviewable status: 5 of 6 files reviewed, 5 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, @mindhog, and @weiminyu)


java_common.gradle, line 78 at r3 (raw file):

// - Format in place: ./gradlew spotlessApply
spotless {
  java {

what does this part do?


java-format/google-java-format-git-diff.sh, line 26 at r3 (raw file):

# the fork point and the HEAD of the current branch.

read -r -d '' USAGE << EOM

does USAGE="...." not work here for some reason?


java-format/google-java-format-git-diff.sh, line 37 at r3 (raw file):

EOM

SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"

Can you do SCRIPT_DIR="$(realpath $(dirname $0))" instead?


java-format/google-java-format-git-diff.sh, line 41 at r3 (raw file):

function callGoogleJavaFormatDiff() {
  local forkPoint
  forkPoint=$(git merge-base --fork-point origin/master) || exit $?

Instead of the "|| exit $?" clauses can you do "set -e" at the beginning of the script?

Only apply Google Java format to changed regions
Diffs are relative to origin/master.

Three tasks are added:
- javaIncrementalFormatCheck is added to the build workflow, and
  will abort build if format violations are found.
- javaIncrementalFormatApply needs to be manually invoked to correct
  format violations, the same behavior as spotlessApply.
- javaIncrementalFormatDryRun shows the changes that would happen if
  javaIncrementalFormatApply is invoked.

These tasks work from the root directory and process the buildSrc directory
too.

The Spotless Java config is removed.
@weiminyu
Copy link
Collaborator Author

left a comment

Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, and @mindhog)


java_common.gradle, line 78 at r3 (raw file):

Previously, mindhog (Michael Muller) wrote…

what does this part do?

It was intended to reformat the entire repo, but the 'target' was misconfigured and does nothing.


java-format/google-java-format-git-diff.sh, line 26 at r3 (raw file):

Previously, mindhog (Michael Muller) wrote…

does USAGE="...." not work here for some reason?

Thought there was a problem with line break but it went away.


java-format/google-java-format-git-diff.sh, line 37 at r3 (raw file):

Previously, mindhog (Michael Muller) wrote…

Can you do SCRIPT_DIR="$(realpath $(dirname $0))" instead?

Done.


java-format/google-java-format-git-diff.sh, line 41 at r3 (raw file):

Previously, mindhog (Michael Muller) wrote…

Instead of the "|| exit $?" clauses can you do "set -e" at the beginning of the script?

Done.

@weiminyu
Copy link
Collaborator Author

left a comment

Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, and @mindhog)


java-format/google-java-format-git-diff.sh, line 26 at r3 (raw file):

Previously, weiminyu (Weimin Yu) wrote…

Thought there was a problem with line break but it went away.

Done.

@weiminyu weiminyu requested a review from mindhog Aug 28, 2019

@lgtm-com

This comment has been minimized.

Copy link

commented Aug 28, 2019

This pull request introduces 1 alert when merging d184c90 into 844c470 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None
@jianglai

This comment has been minimized.

Copy link
Member

commented Aug 28, 2019

This pull request introduces 1 alert when merging d184c90 into 844c470 - view on LGTM.com

new alerts:

  • 1 for Testing equality to None

@weiminyu can you make the suggested change?

@mindhog
Copy link
Member

left a comment

Reviewed 1 of 5 files at r2, 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys, @gbrodman, @hstonec, and @jianglai)

@gbrodman
Copy link
Collaborator

left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, @jianglai, and @weiminyu)


build.gradle, line 337 at r4 (raw file):

}

rootProject.ext {

Do you think it might be worth putting this in a separate Gradle file?

@weiminyu
Copy link
Collaborator Author

left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @jianglai)


build.gradle, line 337 at r4 (raw file):

Previously, gbrodman wrote…

Do you think it might be worth putting this in a separate Gradle file?

Maybe later, when we have more util snippets.

Only apply Google Java format to changed regions
Diffs are relative to origin/master.

Three tasks are added:
- javaIncrementalFormatCheck is added to the build workflow, and
  will abort build if format violations are found.
- javaIncrementalFormatApply needs to be manually invoked to correct
  format violations, the same behavior as spotlessApply.
- javaIncrementalFormatDryRun shows the changes that would happen if
  javaIncrementalFormatApply is invoked.

These tasks work from the root directory and process the buildSrc directory
too.

The Spotless Java config is removed.

@weiminyu weiminyu requested review from gbrodman and mindhog Aug 29, 2019

@mindhog
Copy link
Member

left a comment

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys, @gbrodman, @hstonec, and @jianglai)

@gbrodman
Copy link
Collaborator

left a comment

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys, @gbrodman, @hstonec, and @jianglai)

@CydeWeys
Copy link
Member

left a comment

I'm gonna punt to Gus on this review. But if this is working, let's go ahead with it.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CydeWeys, @gbrodman, @hstonec, and @jianglai)

@weiminyu weiminyu merged commit b5ef99a into google:master Aug 29, 2019

5 of 7 checks passed

code-review/reviewable 1 discussion left (gbrodman, hstonec, jianglai)
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
LGTM analysis: Python No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details

@weiminyu weiminyu deleted the weiminyu:diff-only-format branch Aug 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.