gnodet commented on code in PR #1909:
URL: https://github.com/apache/maven-resolver/pull/1909#discussion_r3373419422
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/repository/ChainedWorkspaceReader.java:
##########
@@ -47,19 +48,17 @@ public final class ChainedWorkspaceReader implements
WorkspaceReader {
* @see #newInstance(WorkspaceReader, WorkspaceReader)
*/
public ChainedWorkspaceReader(WorkspaceReader... readers) {
- if (readers != null) {
- Collections.addAll(this.readers, readers);
- }
-
+ ArrayList<WorkspaceReader> list =
+
Arrays.stream(readers).filter(Objects::nonNull).collect(Collectors.toCollection(ArrayList::new));
Review Comment:
Nit: note that this slightly changes the null-handling behaviour compared to
the previous code. The old `Collections.addAll(this.readers, readers)` would
silently accept a `null` varargs array (treating it as empty), whereas
`Arrays.stream(readers)` will throw `NullPointerException` if `readers` itself
is `null`. On the other hand, individual `null` elements in the array were
previously included (causing an NPE later when iterating), while now they are
filtered out. Worth documenting these behaviour changes in Javadoc or a
comment, even if `null` inputs aren’t part of the intended public contract.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/repository/ChainedWorkspaceReader.java:
##########
@@ -47,19 +48,17 @@ public final class ChainedWorkspaceReader implements
WorkspaceReader {
* @see #newInstance(WorkspaceReader, WorkspaceReader)
*/
public ChainedWorkspaceReader(WorkspaceReader... readers) {
- if (readers != null) {
- Collections.addAll(this.readers, readers);
- }
-
+ ArrayList<WorkspaceReader> list =
+
Arrays.stream(readers).filter(Objects::nonNull).collect(Collectors.toCollection(ArrayList::new));
Review Comment:
Minor style point: since the collected list is immediately wrapped in
`Collections.unmodifiableList`, the exact `ArrayList` implementation doesn’t
matter. You could simplify to `.collect(Collectors.toList())`, or, since
maven-resolver targets Java 11+, even use
`Stream.of(readers).filter(Objects::nonNull).collect(Collectors.toUnmodifiableList())`
(Java 10+).
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/repository/ChainedWorkspaceReader.java:
##########
@@ -104,22 +105,20 @@ public List<String> findVersions(Artifact artifact) {
return Collections.unmodifiableList(new ArrayList<>(versions));
}
+ @Override
public WorkspaceRepository getRepository() {
- Key key = new Key(readers);
- if (!key.equals(repository.getKey())) {
- repository = new WorkspaceRepository(repository.getContentType(),
key);
- }
return repository;
Review Comment:
The simplification here hinges on the assumption that
`WorkspaceReader.getRepository()` always returns a repository whose `getKey()`
is stable for the lifetime of the reader. `WorkspaceRepository` itself is
indeed immutable (its `key` field is `final`), but `WorkspaceReader` is an
interface — an implementation could theoretically return a *different*
`WorkspaceRepository` instance with a different key on each invocation. The old
code defended against that case by re-checking the key on every call. Would be
good to document this assumption, e.g. with a comment like `// Assumes
WorkspaceReader.getRepository() returns a stable (immutable) repository for the
reader’s lifetime`.
--
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]
