findepi commented on code in PR #10474:
URL: https://github.com/apache/iceberg/pull/10474#discussion_r1641073768
##########
baseline.gradle:
##########
@@ -60,8 +63,7 @@ subprojects {
spotless {
java {
target 'src/main/java/**/*.java', 'src/test/java/**/*.java',
'src/jmh/java/**/*.java', 'src/integration/java/**/*.java'
- // we use an older version of google-java-format that is compatible
with JDK 8
- googleJavaFormat("1.7")
+ googleJavaFormat("1.17.0")
Review Comment:
thank you for your comment, @nastra !
> using JDK8 it would produce one formatting and a different formatting when
using JDK11+ (which you can also see in this PR, where the formatting changed).
in this PR formatting changed, because GJF version changed
> Back then I looked into why it behaves the way it does and it was due to
different JDK versions might pick up a different GJF version and so we had to
pin the version to the latest one that was supported by JDK8.
that explains everything, thanks!
Indeed, formatting is apparently dependent on GJF version and GJF version
should be pinned.
on the code from this PR i run the following
```
JAVA_HOME=/Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home
./gradlew spotlessApply
JAVA_HOME=/Library/Java/JavaVirtualMachines/zulu-17.jdk/Contents/Home
./gradlew spotlessApply
JAVA_HOME=/Library/Java/JavaVirtualMachines/zulu-21.jdk/Contents/Home
./gradlew spotlessApply
```
and none of these commands resulted in code changes. The formatting rules
seem consistent and remain independent from the JDK version the build runs with.
> Typically whoever does a release is expected to run the release script
using JDK8 locally btw.
This is what i was assuming, thanks for confirming this!
Note that with changes here, the build continues to work with JDK 8.
It just doesn't offer code style checks under JDK 8, but the person running
release doesn't need them. The release goes off `main` branch where all the
checks were already done by the CI.
If a person running a release wanted to do some code changes before creating
the release build -- which sounds wrong to me -- they would still need to
re-run all other CI checks, which involve JDK versions 8, 11 and 17 (currently;
and 21 should be added to the mix).
> I hope that helps in understanding the context as to why we had to pin the
GJF version.
Absolutely! So with this PR, the GJF version remains pinned. I hope this
clarifies the concerns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
