[MASSEMBLY-832] Consider dependency optional attribute when gather de…#49
[MASSEMBLY-832] Consider dependency optional attribute when gather de…#49zabetak wants to merge 2 commits intoapache:masterfrom
Conversation
src/main/mdo/assembly-component.mdo
Outdated
| <name>useOptionalDependencies</name> | ||
| <version>2.1.0+</version> | ||
| <type>boolean</type> | ||
| <defaultValue>false</defaultValue> |
There was a problem hiding this comment.
I wonder if the defallt here should be true to avoid changing existing behavior.
There was a problem hiding this comment.
I changed the default to true to retain the previous behavior.
| <field> | ||
| <name>useOptionalDependencies</name> | ||
| <version>2.1.0+</version> | ||
| <type>boolean</type> |
There was a problem hiding this comment.
Is this a boolean or a list? What if someone wants to include some optional dependencies and not others?
There was a problem hiding this comment.
They could possibly achieve this by declaring a separate dependencySet. It doesn't make much sense to make this very specific cause in that case we could use the existing include/exclude options.
| private Set<Artifact> resolveDependencyArtifacts( DependencySet dependencySet, Set<Artifact> artifacts ) throws Exception | ||
| { | ||
| MavenProject project = new MavenProject(new Model()); | ||
| Logger logger = new ConsoleLogger(Logger.LEVEL_DEBUG, "test"); |
There was a problem hiding this comment.
no logging at all please. Use a SilentLogger
There was a problem hiding this comment.
Removed the logger as part of the rebase.
|
Resolve #1056 |
|
Thanks for the review @elharo ! I rebased the PR against master, addressed all review comments, and also added a new integration test for the new functionality. |
|
|
||
| @Test | ||
| public void testGetDependencyArtifacts_ShouldFilterOptionalArtifactsExplicitly() throws Exception { | ||
| Artifact am1 = mockArtifact(1); |
There was a problem hiding this comment.
am1 --> artifact1
am2 --> artifact2
here and elsewhere avoid abbreviations
| } | ||
|
|
||
| private static Artifact mockArtifact(int id) { | ||
| Artifact a = mock(Artifact.class); |
|
|
||
| @Test | ||
| public void testGetDependencyArtifacts_ShouldNotFilterOptionalArtifactsByDefault() throws Exception { | ||
| Artifact am1 = mockArtifact(1); |
There was a problem hiding this comment.
no need to sue mocks here. You can simply create a DefaultArtifact.
3 participants
https://issues.apache.org/jira/browse/MASSEMBLY-832
Maven allows to declare dependencies as optional and this is important for various reasons (e.g., avoiding licensing problems).
This change provides a convenient way to include/exclude optional dependencies in a
dependencySet.By default optional dependencies are excluded from the
dependencySet. An optional dependency shouldn't be packaged, distributed, or transitively inherited unless specified explicitly so it makes sense to exclude them by default.Following this checklist to help us incorporate your contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MASSEMBLY-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MASSEMBLY-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under the Apache License Version 2.0, January 2004 you have to acknowledge this by using the following check-box.