Conversation
slawekjaranowski
left a comment
There was a problem hiding this comment.
Please remove commit [ENFORCER-939] Support enforcer.skip without a POM from PR
496f1b2 to
e14820e
Compare
Done |
slawekjaranowski
left a comment
There was a problem hiding this comment.
Thanks for working on this! A few remarks before we can merge.
1. The notation should be consistent with dependency:tree
Maven already has an established notation for a managed version — maven-dependency-tree (VerboseDependencyNode) renders it in the verbose dependency tree as:
+- org.example:childA:jar:1.0.0:compile (version managed from 2.0.0)
Rather than introducing a third variant, let's reuse exactly that, so a user sees the same wording in mvn dependency:tree -Dverbose and in the enforcer output:
+-org.example:childA:1.0.0 (version managed from 2.0.0)
This affects the rule, the unit test and require-upper-bound-dependencies-managed_failure/verify.groovy.
2. Please add a new test instead of modifying the existing one
RequireUpperBoundDepsTest#testRule currently covers the plain conflict between two dependency paths and asserts that both versions show up in the message. After adding withPremanagedVersion("2.0.0") to the first child, that node alone already triggers the conflict and the assertion no longer verifies the two-path output — so the original case is effectively no longer covered.
Could you please restore testRule to its previous form and add a separate test (e.g. testManagedVersion) for the managed case? Then both behaviours stay covered. DependencyNodeBuilder#withPremanagedVersion is a good addition and can stay as is.
3. Documentation text is placed inside the code block
In enforcer-rules/src/site/markdown/requireUpperBoundDeps.md.vm the new sentence was inserted before the closing fence, so it will be rendered as part of the sample log output. It has to go after the closing ```.
Also, the sample log in that block contains no managed dependency at all, so the explanation has nothing to refer to. A short, separate example showing a managed dependency would be much clearer.
4. Unrelated changes
buildErrorMessage() only gained blank lines — please drop them to keep the diff focused on one change.
My bad I was busy last week,I have changed the things as you asked. |
Description
Improve the
RequireUpperBoundDepserror message when a dependency version is managed to a different version.Previously, managed dependencies were displayed like:
org.example:childA:1.0.0 (managed) <-- org.example:childA:2.0.0The meaning of the arrow notation is not immediately clear, making it difficult to determine which version was selected and which version was requested.
This changes the message to:
org.example:childA:1.0.0 (managed; requested org.example:childA:2.0.0)This makes the relationship between the managed version and the requested version explicit.
Changes
RequireUpperBoundDepsto use clearer managed-version error message wording.DependencyNodeBuilderto create dependency nodes with a premanaged version for testing.managed; requestednotation.Testing
mvn verify— passedmvn -Prun-its verify— passedFixes #937
Following this checklist to help us incorporate your contribution quickly and easily:
Your pull request should address just one issue, without pulling in other changes.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Each commit in the pull request should have a meaningful subject line and body.
Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
Run
mvn verifyto make sure basic checks pass.You have run the integration tests successfully (
mvn -Prun-its verify).I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004.
In any other case, please file an Apache Individual Contributor License Agreement.