-
Notifications
You must be signed in to change notification settings - Fork 371
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
Alternative ExcludeTransitiveDependency
#3689
base: main
Are you sure you want to change the base?
Conversation
Note: This is still WIP. I don't think a lot is missing though. |
} | ||
|
||
@Test | ||
void excludeAllTransitiveDependencies() { |
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.
Normally when I see multiple of these excludes in the manner shown here, it makes me start to wonder if a global exclude isn't a better option.
I could see a global exclude dependency recipe satisfying that case though as another potential option.
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.
That is what this recipe should be doing, when no configuration
option is set. Does that make sense?
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'll clarify the difference that I'm talking about a little bit.
I would consider each of these as a dependency-specfic exclusion even if the recipe applies said exclusion to all dependencies.
A global exclusion looks like:
configurations {
all {
exclude group: "commons-collections", module: "commons-collections"
// Add additional statements for other modules
// exclude group: "commons-lang", module: "commons-lang"
}
}
dependencies {
implementation("commons-beanutils:commons-beanutils:1.9.4")
implementation("com.opencsv:opencsv:4.6")
testImplementation("commons-beanutils:commons-beanutils:1.9.4")
testImplementation("com.opencsv:opencsv:4.6")
}
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.
@shanman190 This looks really good! Possibly we don't even need any configuration-specific exclusions, as I think that will be a rather esoteric use case. Possibly we could have some kind of all
option to control whether to do it this way or whether to add exclusions to the individual dependencies (defaulting to modify the all
configuration).
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 personally see both as being useful. The use cases in front of someone that needs to add the exclusion often guide to which implementation to use, but if it were me I'd probably make the following edits to support both styles.
- Rename
groupId
toexcludeGroupId
- Rename
artifactId
toexcludeArtifactId
- Introduce an optional
groupId
(more in a moment) - Introduce an optional
artifactId
(more in a moment)
When groupId
and artifactId
are empty, we would create a global exclusion either in all
if configuration
is null or blank, otherwise in the specified configuration
. When groupId
and artifactId
are present, then we add the exclusion only to the targeted dependencies using the same selection criteria.
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.
Also technically only one of excludeGroupId
or excludeArtifactId
is required. So you can add exclusions like the following:
configurations {
all {
exclude group: "commons-collections"
exclude module: "commons-collections"
}
}
dependencies {
implementation("commons-beanutils:commons-beanutils") {
exclude group: "commons-collections"
exclude module: "commons-collections"
}
}
No description provided.