-
Notifications
You must be signed in to change notification settings - Fork 408
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
Support using type-safe dependency accessors in ShadowJar.dependencies
#1322
Conversation
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.
Code Review
The pull request updates the dependency resolution logic in AbstractDependencyFilter.kt
to handle Provider
and DependencyValueSource
types. This change aims to improve the flexibility of dependency filtering. The addition to lint-baseline.xml
suppresses a lint error related to internal Gradle API usage.
Summary of Findings
- Internal Gradle API Usage: The code introduces a dependency on
org.gradle.api.internal.catalog.DependencyValueSource
, which is an internal Gradle API. While the lint baseline is updated to suppress the warning, it's crucial to understand the implications of using internal APIs, as they are subject to change without notice and can break the plugin in future Gradle versions. Consider if there are public APIs that can achieve the same result. - Nullability of
dependencyNotation.obtain()
: The code usesrequireNotNull(dependencyNotation.obtain())
. While this prevents a potential NullPointerException, it would be useful to understand the conditions under whichdependencyNotation.obtain()
can return null, and whether a more graceful handling mechanism is possible.
Merge Readiness
The pull request introduces changes that touch internal Gradle APIs. While the immediate functionality might be correct, the long-term maintainability and compatibility with future Gradle versions should be carefully considered. Given the use of internal APIs, I recommend a thorough review by someone with deep knowledge of Gradle internals. I am unable to approve this pull request, and recommend that others review and approve this code before merging. At a minimum, the high severity issue should be addressed before merging.
src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/AbstractDependencyFilter.kt
Outdated
Show resolved
Hide resolved
…pport-type-safe-accessors
ShadowJar.dependencies
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.
PR Overview
This PR adds support for using type-safe dependency accessors in ShadowJar.dependencies, as noted in the changelog update.
- Introduces a changelog entry for the new feature.
Reviewed Changes
File | Description |
---|---|
docs/changes/README.md | Updated changelog with a new entry for type-safe dependency accessor usage. |
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
…pe-safe-accessors
Closes #923.