Test using type-safe accessors in ShadowJar.dependencies
#1327
Conversation
This file contains 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
* A new FilteringCachingTest * Tweak contains checks * Use assertCompositeExecutions * Remove SUCCESS check for canRegisterCustomShadowJarTask * Remove FAILED check for failBuildIfProcessingBadJar * Remove FAILED check for honorDuplicatesStrategyWithThrowing * Remove configurationCachingOfConfigurationsIsUpToDate as no inputs changed * Fix style * Reduce all assertions
* Test `relocateAllPackagesButSomeone` * Use ParameterizedTest * Cleanups * Rename
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces type-safe accessors in ShadowJar.dependencies and includes functional tests to verify the behavior. The changes look good overall, but there are a few areas that could be improved for clarity and maintainability.
Summary of Findings
- Settings script modification: The settings script is modified to enable the
TYPESAFE_PROJECT_ACCESSORSfeature preview. This is necessary for the tests, but it's important to ensure that this doesn't have unintended side effects on other parts of the project. - Parameterized tests: The introduction of parameterized tests with
ValueSourceis a good way to test both with and without type-safe accessors. However, the nameuseAccessorcould be more descriptive. - Dependency resolution management: The addition of
dependencyResolutionManagementin thedependOnAndExcludeArtifactDfunction seems specific to the test case. Consider if this can be made more generic or if it's truly needed for this specific test.
Merge Readiness
The pull request is almost ready for merging. Addressing the comments regarding the test parameter name and the dependency resolution management setup would improve the code quality. I am unable to directly approve the pull request, and users should have others review and approve this code before merging.
src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/FilteringTest.kt
Show resolved
Hide resolved
src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/FilteringTest.kt
Show resolved
Hide resolved
src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/FilteringTest.kt
Show resolved
Hide resolved
src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/FilteringTest.kt
Show resolved
Hide resolved
|
They are not working for main branch now. |
Goooler
changed the base branch from
main
to
g/20250307/support-type-safe-accessors
Goooler
merged commit 970f537
into
g/20250307/support-type-safe-accessors
1 of 7 checks passed
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
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.
Refs #1322.