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

fixed usage of PomDownloader without settings #425

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

joanvr
Copy link
Contributor

@joanvr joanvr commented Aug 24, 2023

What's changed?

Moved some code to inner MavenVisitor and passing the settings and profiles to the MavenPomDownloader class

What's your motivation?

Without proper usage of maven settings, some people might find out that their configured repositories are ignored.

Checklist

  • I've used the IntelliJ IDEA auto-formatter on affected files

@joanvr joanvr requested a review from timtebeek August 24, 2023 11:30
@joanvr joanvr self-assigned this Aug 24, 2023
@joanvr joanvr added the bug Something isn't working label Aug 24, 2023
@timtebeek
Copy link
Contributor

I'm wondering if we should update MavenPomDownloader to deprecate the old constructors, and add a new one that expects (or calls) getResolutionResult(). What would you think about that?

@joanvr
Copy link
Contributor Author

joanvr commented Aug 24, 2023

I'm wondering if we should update MavenPomDownloader to deprecate the old constructors, and add a new one that expects (or calls) getResolutionResult(). What would you think about that?

Maybe there are scenarios where the current constructor without taking into consideration the settings is needed? I might know better when I explore all the usages that we found...

@timtebeek
Copy link
Contributor

I'm wondering if we should update MavenPomDownloader to deprecate the old constructors, and add a new one that expects (or calls) getResolutionResult(). What would you think about that?

Maybe there are scenarios where the current constructor without taking into consideration the settings is needed? I might know better when I explore all the usages that we found...

Sure! Let me know what you find; the reason I'd change the API around PomDownloader is that I don't want to have to look out for that in reviews in the future. Not sure if there's ever a good case to skip it, but I wouldn't think there's a good reason to bypass the user configured preferences.

@sambsnyd sambsnyd merged commit a228894 into main Sep 13, 2023
1 check passed
@sambsnyd sambsnyd deleted the fix/pom-downloader-without-settings branch September 13, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants