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

Skip dependency resolution option for MavenParser skips parent resolution as well #3984

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jkschneider
Copy link
Member

Improves the skipping of dependency resolution on MavenParser by:

  • Allowing to skip parent resolution as well
  • Moving the skip option to MavenExecutionContextView which is already supplied to the parsing phase.

@@ -343,7 +343,9 @@ class Resolver {
MavenPomDownloader downloader;

public ResolvedPom resolve() throws MavenDownloadingException {
resolveParentsRecursively(requested);
if (!MavenExecutionContextView.view(ctx).getSkipDependencyResolution()) {
resolveParentsRecursively(requested);
Copy link
Contributor

Choose a reason for hiding this comment

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

To detect Java versions in the CLI it'd be helpful to still resolve parent poms without resolving dependencies. That aligns with how we've seen customers use archetypes to set the Java version. Would it make sense to make resolving parent poms a separate option?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not liking the options if that's the case. Almost would rather a general purpose dependency resolution interceptor pattern than an array of very specific options.

@jkschneider jkschneider marked this pull request as draft February 7, 2024 17:31
@jkschneider
Copy link
Member Author

Reassessing whether this makes sense.

@knutwannheden
Copy link
Contributor

Reassessing whether this makes sense.

My reasoning for using the parser builder was that we during parsing typically reuse the same execution context and that the parsers return a stream, where the sources get lazily parsed. So the client would theoretically have to make sure the execution context stores the correct setting at the time it "pulls" the corresponding source from the stream.

This is a bit theoretical, as we won't be using this specific option anywhere where this would be a problem. I was just a bit worried that it might set a bad precedence for other parser options, but this was possibly unjustified.

@knutwannheden
Copy link
Contributor

Coincidentally there was this discussion regarding the encoding to be used by the parser just earlier today: openrewrite/rewrite-maven-plugin#735 (comment)

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.

None yet

3 participants