-
Notifications
You must be signed in to change notification settings - Fork 466
Ensure the Maven and Gradle DependencyInsights recipes treat dependencies coming in through various paths the same #5882
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
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/java/org/openrewrite/gradle/search/DependencyInsight.java
- lines 22-22
Yep, looking good! I am only wondering about the maven counterpart. If maven's recipe can also have this same problem, I would add it in this PR as well. |
rewrite-gradle/src/main/java/org/openrewrite/gradle/search/DependencyInsight.java
Outdated
Show resolved
Hide resolved
…endencyInsight.java Co-authored-by: Jacob van Lingen <[email protected]>
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/java/org/openrewrite/gradle/marker/GradleDependencyConfiguration.java
- lines 33-33
- lines 271-271
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’m pretty sure this is a non-goal in fact. Dependencies can come to be in multiple ways in a file. And there is utility in knowing how many times a dependency is included
As discussed in Slack: we'll instead change the Maven recipe to match Gradle, as opposed to changing the Gradle recipe to match Maven. We need to update the tests accordingly and the Maven DependencyInsight recipe. In the future we might add a column to distinguish between the data table rows produced. |
Dependencies can come in through various paths; at present we do not show the path of how dependencies come in in the data table rows. This may lead to duplicate rows, which we accept until we add a column to differentiate between rows (not a goal for this PR).
Before
We see a difference between Maven and Gradle DependencyInsight recipes: the Gradle recipe adds identical rows for the varying paths, whereas the Maven recipe does not add those identical rows.
After
Both recipes add rows for each path that might bring in a dependency, even if those rows might be identical.