-
Notifications
You must be signed in to change notification settings - Fork 56
SpotlessApply uses by default the Java formatter #1243
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
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
d44a3fa
tests with java versions
crogoz e4a42d1
fix
crogoz 070c5cf
.
crogoz 17e9bdd
Add generated changelog entries
svc-changelog 6021843
add readme explanation
crogoz 66c2d7c
Merge branch 'cr/spotless-uses-java-if-21' of github.com:palantir/pal…
crogoz b9b5073
Update README.md
crogoz 81ff9ae
Update README.md
crogoz 9cd9bc2
use ClasspathAddingInitScriptBuilder
crogoz ea77c75
.
crogoz 7d30bfb
fix
crogoz 065d9c5
cleanup
crogoz 113ee73
fix
crogoz 1dfcc88
fix
crogoz 22132dc
fix waitFor
crogoz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
type: fix | ||
fix: | ||
description: SpotlessApply uses Java 21 version | ||
links: | ||
- https://github.com/palantir/palantir-java-format/pull/1243 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I move this to https://github.com/palantir/gradle-plugin-testing ? gradle-jdks needs to do the same thing in order to run on multiple Java versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what it's supposed to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose what I don't understand is why you need this at all? I thought nebula does this for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to test how the task works against Java 17 and Java 21. So, I am using the
gradle-jdks automanagement workflow
to set-up the JDKs but then I also need to run./gradlew
instead of the classicnebula runTask
. When using nebularunTask
the plugin is injected automatically, but now I need to set it on my own. This is whatgetBuildPluginClasspathInjector
does - it adds the files of the plugin to the test classpath.I have the same setup in
gradle-jdks
for tests - so I was wondering if it makes sense to move this logic in the testing plugin.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I thought the tooling api invoked gradlew, interesting!
you could move it to gradle-plugin-testing, it's a bit niche, but I don't see why not.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a nicer way to do this, is probably to reuse some of the stuff in Nebula, it makes your life a lot easier I think:
ClasspathAddingInitScriptBuilder
to write the classpath in an init script formgradlew
. i.e../gradlew <task> -i init-script
I think the current approach - I also mentioned something similar in the testing plugin PRs - detracts from the test by adding a bunch of cruft that's not actually important to know for the test, but is unfortunately required for the test to pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also means, that you can hide all of this behind a
runGradlew
method and not have to pollute the test with it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 9cd9bc2
however, I still need to keep this method, if I use the
ClassLoader
to add all the files loaded by default - the test will fail hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An aside: I think you could probably ship this method +
runTasksViaGradlew
into gradle-testing-plugin or a new thing. It'd just look very similar toIntegrationSpec
or something.