Skip to content

fix: code should not rely on MavenCoordinate equals/hashcode#2135

Merged
maxandersen merged 1 commit intojbangdev:mainfrom
maxandersen:noequalsmvnc
Aug 7, 2025
Merged

fix: code should not rely on MavenCoordinate equals/hashcode#2135
maxandersen merged 1 commit intojbangdev:mainfrom
maxandersen:noequalsmvnc

Conversation

@maxandersen
Copy link
Copy Markdown
Collaborator

While doing #2133 i spotted that ModuleUtil relied on MavenCoordinate to be used as key in list - that wont do what you most likely want given a mavencoordinate is more like a "request for a dependency" so it can have version ranges in it and soon scope info.

So I'm isolating this fix as I believe its a bug on its own right and to not make scope deps fix having too many things in it.

@maxandersen maxandersen marked this pull request as ready for review August 2, 2025 12:55
@maxandersen maxandersen requested a review from quintesse August 2, 2025 12:55
@quintesse
Copy link
Copy Markdown
Contributor

Sure, seems logical.

Not okaying yet because a) I don't understand the module tests you added/changed and b) the commented out code should be removed, right? (A comment saying we shouldn't have equals/hash + reason is probably a good idea , so we don't fall into this trap in the future)

@maxandersen
Copy link
Copy Markdown
Collaborator Author

The module test is a dependency with version range.

All the exisiting tests failed after I added attribute syntax support and first I thought i broke something but then I realized even if we implemented equals/hashxode to make the tests passes code using version ranges would still fail as resolved maven coordinate has no version ranges and thus wouldn't be equals. And contains would still fail. Hence why i made it explicitly used managed key (g:a:type) which is what module util code should have used originally anyway.

@quintesse
Copy link
Copy Markdown
Contributor

Ok, understood, LGTM (still, you'll remove the commented code, right? :-) )

quintesse
quintesse previously approved these changes Aug 3, 2025
@maxandersen
Copy link
Copy Markdown
Collaborator Author

@quintesse ok, moved the description to top and removed the commented out equals/hashcode implementations

@maxandersen maxandersen merged commit bd32741 into jbangdev:main Aug 7, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants