gnodet commented on code in PR #909:
URL: https://github.com/apache/maven/pull/909#discussion_r1048744963
##########
maven-core/src/main/java/org/apache/maven/settings/SettingsUtils.java:
##########
@@ -51,6 +52,25 @@ public static Settings merge(Settings dominant, Settings
recessive, String reces
return new MavenSettingsMerger().merge(dominant, recessive,
recessiveSourceLevel);
}
+ /**
+ * @param dominant
+ * @param recessive
+ * @param recessiveSourceLevel
+ * @deprecated please use {@link #merge(Settings, Settings, String)}
+ */
+ @Deprecated
+ public static void merge(
+ org.apache.maven.settings.Settings dominant,
+ org.apache.maven.settings.Settings recessive,
+ String recessiveSourceLevel) {
+
+ if (dominant == null || recessive == null) {
+ return;
+ }
+
+ dominant.delegate = new SettingsMerger().merge(dominant.getDelegate(),
recessive.getDelegate(), true, null);
Review Comment:
> > The only non immutable bit in the m4 `Settings` is the `sourceLevel`
which I also have no idea where/why/how/if it's used.
>
> Exactly, and `sourceLevel` can be set once - it is a reason why I used
`SettingsMerger`.
Ah, I see.
> m-invoker-p has a hack for it:
https://github.com/apache/maven-invoker-plugin/blob/bfb75f9e52e93478dab710bb7243978c06f48d1a/src/main/java/org/apache/maven/plugins/invoker/AbstractInvokerMojo.java#L1580-L1618
> but only works with Maven3 - 4 has session in delegate field
Ok, I think that's not a real issue, right ? As exception is caught and
forgot. But this would allow to get rid of this code in the future.
> We also can relax of settings `sourceLevel`.
I would be in favour of getting rid of it in the v4 api if it's not used,
and worse, actually worked around.
> By the way we have double implementation `MavenSettingsMerger` and
`SettingsMerger` IMHO we should use one.
I wasn't sure if the results would be the same, given the first one is
hand-crafted and the second one generated.
If that's the case, the easier would be to deprecate `MavenSettingsMerger`
and have it delegated to `SettingsMerger`. The `MavenSettingsMerger` is also
the one used by default in the `DefaultSettingsBuilder`, but we can try and see
if the ITs raise any issues.
Also, if the goal is to restore compatibility, maybe a cleaner way would be
similar to https://github.com/apache/maven/pull/874, i.e. restore the methods
as they were in m3, but delegating to a new v4 similar class. Both PRs are
conflicting but #874 also has additional value and should have been merged
earlier imho.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
