Skip to content
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

When comparing Gradle configurations compare the contents not the reference equality #4700

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lkerford
Copy link
Contributor

When we compare Gradle configurations we are checking for reference equality not the contents. We are missing configuration we should be adding when retrieving the list of extended configurations

@shanman190
Copy link
Contributor

Do the contents of a configuration really need to be compared deeply? All of the marker methods use the same immutable patterns of other LST elements.

@lkerford
Copy link
Contributor Author

Do the contents of a configuration really need to be compared deeply? All of the marker methods use the same immutable patterns of other LST elements.

I do believe so, we are currently failing to match configurations which should be equivalent. This is resulting in the change dependencies recipes failing to update dependencies which should be updated.

@shanman190
Copy link
Contributor

I'm still not totally convinced here.

The various change recipes act upon the LST itself, then when there is a confirmed change update the GradleProject marker.

Ex:
https://github.com/openrewrite/rewrite/blob/main/rewrite-gradle%2Fsrc%2Fmain%2Fjava%2Forg%2Fopenrewrite%2Fgradle%2FChangeDependency.java#L173-L175

Another piece of potentially relevant information is that we do not update the various extendsFrom configurations transitively when a top-level configuration is modified. Partly because we don't fully resolve the dependency graph like is done with rewrite-maven during a recipe execution. But even with not performing these updates and instead relying upon a new LST generation, it shouldn't impact most change recipe executions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants