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

Advice to display Version Catalog aliases #837

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Chasson1992
Copy link

@Chasson1992 Chasson1992 commented Dec 30, 2022

Fix #794

To allow advice to display the Version Catalog aliases we can iterate through each alias and add a mapping of the alias to the dependency in the DependencyHandler#map extension. Followed the recommendations from @daanschipper and @autonomousapps described in the issue.

I didn't add any functional tests, since it's fairly simple and safe, but I'm open to doing so.

A full working repository to demonstrate the issue is located here

buildHealth output before the changes:

Advice for root project
Dependencies which should be removed or changed to runtime-only:
  runtimeOnly("ch.qos.logback:logback-classic:1.2.6") (was implementation)

buildHealth output after the changes:

Advice for root project
Dependencies which should be removed or changed to runtime-only:
  runtimeOnly(libs.logback.classic) (was implementation)

@autonomousapps autonomousapps added the enhancement New feature or request label Jan 3, 2023
@autonomousapps autonomousapps self-requested a review January 3, 2023 01:41
@autonomousapps
Copy link
Owner

Thanks for the PR!

Copy link
Owner

@autonomousapps autonomousapps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you.

Since this is the first feature to directly support the version catalog concept, I think having a functional test is worth doing, even though it does indeed look quite safe. The test's purpose would be more to ensure there isn't a future regression. Let me know if I can provide any pointers here.

@Chasson1992
Copy link
Author

@autonomousapps I started to implement a functional test (mirroring the test harness I created under https://github.com/Chasson1992/DependencyAnalysisTest).

I'm developing on a Windows 10 machine with IntelliJ Ultimate but when running the functionalTest task I'm getting a bunch of expections from:

Caused by: java.io.IOException at FileUtils.java:1344
    Caused by: java.nio.file.FileSystemException at WindowsException.java:92

I've been having a hard time figuring out the issue on this one. Any suggestions? I'll push what I have currently to my fork.

@autonomousapps
Copy link
Owner

I've been having a hard time figuring out the issue on this one. Any suggestions? I'll push what I have currently to my fork.

Can you post more of the stacktrace? TBH I don't develop at all on Windows, so I'm not sure I'll be able to help, but at the least I'd need the full stacktrace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Gradle Version Catalogs if present
2 participants