You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "CrazyHZM (via GitHub)" <gi...@apache.org> on 2025/01/15 16:09:13 UTC
[PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
CrazyHZM opened a new pull request, #2049:
URL: https://github.com/apache/maven/pull/2049
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2049:
URL: https://github.com/apache/maven/pull/2049#issuecomment-2597800926
There are two references to `new DefaultInterpolator()` that can be easily removed:
* https://github.com/apache/maven/blob/f5a407cf79494d90ad1df4aa6336239c542d98e1/impl/maven-impl/src/main/java/org/apache/maven/internal/impl/DefaultSettingsBuilder.java#L83-L85
* https://github.com/apache/maven/blob/f5a407cf79494d90ad1df4aa6336239c542d98e1/impl/maven-impl/src/main/java/org/apache/maven/internal/impl/DefaultToolchainsBuilder.java#L60-L62
We can simply inline those constructors, as they are only used in a few tests. This will move the references to the implementation to the tests instead of the main code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #2049:
URL: https://github.com/apache/maven/pull/2049#discussion_r1924842074
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/extensions/ExtensionConfigurationModule.java:
##########
@@ -36,6 +36,7 @@ public class ExtensionConfigurationModule implements Module {
private final CoreExtensionEntry extension;
private final UnaryOperator<String> callback;
+
private final DefaultInterpolator interpolator = new DefaultInterpolator();
Review Comment:
This component is loaded very early without the full container afaik. This would require an `Interpolator` to be bound to the `ProtoSession` and `ProtoLookup`. Currently the only component added is `ClassWorld`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #2049:
URL: https://github.com/apache/maven/pull/2049#issuecomment-2593704154
I'm not sure we want to change the maven 3 code base too much. I've tried to revert most changes to get back into a compatible state, so if we add a constructor, it might break something. And for not much value, as this code is not really used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #2049:
URL: https://github.com/apache/maven/pull/2049#discussion_r1923746114
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/extensions/ExtensionConfigurationModule.java:
##########
@@ -36,6 +36,7 @@ public class ExtensionConfigurationModule implements Module {
private final CoreExtensionEntry extension;
private final UnaryOperator<String> callback;
+
private final DefaultInterpolator interpolator = new DefaultInterpolator();
Review Comment:
What about this one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "CrazyHZM (via GitHub)" <gi...@apache.org>.
CrazyHZM commented on PR #2049:
URL: https://github.com/apache/maven/pull/2049#issuecomment-2595747640
@gnodet @cstamas I also agree with this and have reverted the compatibility logic of maven3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "CrazyHZM (via GitHub)" <gi...@apache.org>.
CrazyHZM merged PR #2049:
URL: https://github.com/apache/maven/pull/2049
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #2049:
URL: https://github.com/apache/maven/pull/2049#discussion_r1916942252
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/extensions/ExtensionConfigurationModule.java:
##########
@@ -36,7 +37,9 @@ public class ExtensionConfigurationModule implements Module {
private final CoreExtensionEntry extension;
private final UnaryOperator<String> callback;
- private final DefaultInterpolator interpolator = new DefaultInterpolator();
+
+ @Inject
+ private DefaultInterpolator interpolator;
Review Comment:
This is not a component. Just modify the ctor and make it final, and pass it in where instance is created
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #2049:
URL: https://github.com/apache/maven/pull/2049#issuecomment-2593740808
Agreed, the compat/maven-embedder changes should be undone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MNG-8524] DefaultInterpolator should be used by injection [maven]
Posted by "CrazyHZM (via GitHub)" <gi...@apache.org>.
CrazyHZM commented on PR #2049:
URL: https://github.com/apache/maven/pull/2049#issuecomment-2598250938
> There are two references to `new DefaultInterpolator()` that can be easily removed:
>
> * https://github.com/apache/maven/blob/f5a407cf79494d90ad1df4aa6336239c542d98e1/impl/maven-impl/src/main/java/org/apache/maven/internal/impl/DefaultSettingsBuilder.java#L83-L85
> * https://github.com/apache/maven/blob/f5a407cf79494d90ad1df4aa6336239c542d98e1/impl/maven-impl/src/main/java/org/apache/maven/internal/impl/DefaultToolchainsBuilder.java#L60-L62
>
> We can simply inline those constructors, as they are only used in a few tests. This will move the references to the implementation to the tests instead of the main code.
@gnodet Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org