You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2024/09/21 14:54:32 UTC
[PR] Sus method that nullifies model [maven]
cstamas opened a new pull request, #1735:
URL: https://github.com/apache/maven/pull/1735
(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] Probable bug: method that nullifies model [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1735:
URL: https://github.com/apache/maven/pull/1735#discussion_r1771192484
##########
maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelPathTranslator.java:
##########
@@ -116,6 +116,17 @@ private Resource alignToBaseDirectory(Resource resource, Path basedir) {
}
private String alignToBaseDirectory(String path, Path basedir) {
+ String newPath = mayAlignToBaseDirectoryOrNull(path, basedir);
+ if (newPath != null) {
+ return newPath;
+ }
+ return path;
+ }
+
+ /**
+ * Returns aligned path or {@code null} if no need for change.
+ */
+ private String mayAlignToBaseDirectoryOrNull(String path, Path basedir) {
Review Comment:
Remove this new method.
##########
maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelPathTranslator.java:
##########
@@ -107,7 +107,7 @@ private <T> List<T> map(List<T> resources, Function<T, T> mapper) {
private Resource alignToBaseDirectory(Resource resource, Path basedir) {
if (resource != null) {
- String newDir = alignToBaseDirectory(resource.getDirectory(), basedir);
+ String newDir = mayAlignToBaseDirectoryOrNull(resource.getDirectory(), basedir);
if (newDir != null) {
Review Comment:
This check should be changed to `if (!Objects.equals(newDir, resource.getDirectory())`
##########
maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelPathTranslator.java:
##########
@@ -116,6 +116,17 @@ private Resource alignToBaseDirectory(Resource resource, Path basedir) {
}
private String alignToBaseDirectory(String path, Path basedir) {
+ String newPath = mayAlignToBaseDirectoryOrNull(path, basedir);
+ if (newPath != null) {
+ return newPath;
+ }
+ return path;
+ }
+
+ /**
+ * Returns aligned path or {@code null} if no need for change.
+ */
+ private String mayAlignToBaseDirectoryOrNull(String path, Path basedir) {
String newPath = pathTranslator.alignToBaseDirectory(path, basedir);
return Objects.equals(path, newPath) ? null : newPath;
Review Comment:
I think the above line should be removed.
--
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] Probable bug: method that nullifies model [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on PR #1735:
URL: https://github.com/apache/maven/pull/1735#issuecomment-2368024565
> In order to optimise things further, I think the above function needs to return null when nothing has changed. Or the map method could be adapted to check for object identity equality rather than null.
I was thinking to cascade the identity checks, as `map` is used not only for `Resource`.
--
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] Probable bug: method that nullifies model [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1735:
URL: https://github.com/apache/maven/pull/1735#issuecomment-2367989882
> A slight amendment is to modify the method as below (with `null` return value replaced by the old value). It may allow to reuse more existing objects compared to inline equality check in `alignToBaseDirectory(Resource)`, because `alignToBaseDirectory(String)` is also invoked directly elsewhere.
>
> ```java
> /**
> * Returns a path relocated to the given base directory. If the result of this operation
> * is the same path as before, then this method returns the old {@code path} instance.
> * It is okay for the caller to compare the {@link String} instances using the identity
> * comparator for detecting changes.
> *
> * @param path the path to relocate, or {@code null}
> * @param basedir the new base directory
> * @return relocated path, or {@code null} if the given path was null
> */
> private String alignToBaseDirectory(String path, Path basedir) {
> String newPath = pathTranslator.alignToBaseDirectory(path, basedir);
> return Objects.equals(path, newPath) ? path : newPath;
> }
> ```
>
> And replace null check by identity check:
>
> ```java
> /**
> * Returns a resource with all properties identical to the given resource, except the paths
> * which are resolved according the given {@code basedir}. If the paths are unchanged, then
> * this method returns the previous instance.
> *
> * @param resource the resource to relocate, or {@code null}
> * @param basedir the new base directory
> * @return relocated resource, or {@code null} if the given resource was null
> */
> @SuppressWarnings("StringEquality") // Identity comparison is ok in this method.
> private Resource alignToBaseDirectory(Resource resource, Path basedir) {
> if (resource != null) {
> String oldDir = resource.getDirectory();
> String newDir = alignToBaseDirectory(oldDir, basedir);
> if (newDir != oldDir) {
> return resource.withDirectory(newDir);
> }
> }
> return resource;
> }
> ```
Good point. In order to optimise things further, I think the above function needs to return `null` when nothing has changed.
--
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] Probable bug: method that nullifies model [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1735:
URL: https://github.com/apache/maven/pull/1735#issuecomment-2367876660
Originally, the code had the following:
```
private String alignToBaseDirectory(String path, Path basedir) {
return pathTranslator.alignToBaseDirectory(path, basedir);
}
```
without the null return value.
We should restore this behavior and inline the equality check into the caller method.
--
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] Probable bug: method that nullifies model [maven]
Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on PR #1735:
URL: https://github.com/apache/maven/pull/1735#issuecomment-2367912140
A slight amendment is to modify the method as below (with `null` return value replaced by the old value). It may allow to reuse more existing objects compared to inline equality check in `alignToBaseDirectory(Resource)`, because `alignToBaseDirectory(String)` is also invoked directly elsewhere.
```java
/**
* Returns a path relocated to the given base directory. If the result of this operation
* is the same path as before, then this method returns the old {@code path} instance.
* It is okay for the caller to compare the {@link String} instances using the identity
* comparator for detecting changes.
*
* @param path the path to relocate, or {@code null}
* @param basedir the new base directory
* @return relocated path, or {@code null} if the given path was null
*/
private String alignToBaseDirectory(String path, Path basedir) {
String newPath = pathTranslator.alignToBaseDirectory(path, basedir);
return Objects.equals(path, newPath) ? path : newPath;
}
```
And replace null check by identity check:
```java
/**
* Returns a resource with all properties identical to the given resource, except the paths
* which are resolved according the given {@code basedir}. If the paths are unchanged, then
* this method returns the previous instance.
*
* @param resource the resource to relocate, or {@code null}
* @param basedir the new base directory
* @return relocated resource, or {@code null} if the given resource was null
*/
@SuppressWarnings("StringEquality") // Identity comparison is ok in this method.
private Resource alignToBaseDirectory(Resource resource, Path basedir) {
if (resource != null) {
String oldDir = resource.getDirectory();
String newDir = alignToBaseDirectory(oldDir, basedir);
if (newDir != oldDir) {
return resource.withDirectory(newDir);
}
}
return resource;
}
```
--
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