gnodet-bot commented on code in PR #1771:
URL: https://github.com/apache/maven-resolver/pull/1771#discussion_r3993923205
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/transport/TransporterFactory.java:
##########
@@ -31,9 +33,39 @@
*/
public interface TransporterFactory {
+ /**
+ * A key for transporter properties.
Review Comment:
⚠️ **Empty marker interface with no usage.** `TransporterPropertyKey` is
declared but never implemented, never referenced as a type parameter, and has
no methods or constants. It's a map key type with no way to create instances —
callers can't construct keys, and no concrete keys are defined anywhere in this
PR.
If this is scaffolding for a future PR, it shouldn't be in the SPI until it
has at least one implementation and one usage site. An empty interface in a
public SPI is a commitment with no payoff.
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/transport/TransporterFactory.java:
##########
@@ -31,9 +33,39 @@
*/
public interface TransporterFactory {
+ /**
+ * A key for transporter properties.
+ * @see #getProperties()
+ */
+ public interface TransporterPropertyKey {}
+
+ /**
+ * Indicates whether this factory can handle the specified repository
protocol.
+ * Even if {@code true} is returned the factory may still refuse to create
a transporter for the given protocol.
+ *
+ * @param repositoryProtocol The repository protocol to check, may be
{@code null}.
+ * @return {@code true} if this factory can potentially handle the
specified repository protocol, {@code false} otherwise.
+ * @see #newInstance(RepositorySystemSession, RemoteRepository)
+ */
+ default boolean canHandle(String repositoryProtocol) {
+ return true;
Review Comment:
⚠️ **Missing `@since` tag.** New public API methods in Apache Maven SPI
interfaces require a `@since` tag for downstream consumers to know when the
method was introduced. Same applies to `getProperties()` below and
`TransporterPropertyKey` above.
```suggestion
/**
* Indicates whether this factory can handle the specified repository
protocol.
* Even if {@code true} is returned the factory may still refuse to
create a transporter for the given protocol.
*
* @param repositoryProtocol The repository protocol to check, may be
{@code null}.
* @return {@code true} if this factory can potentially handle the
specified repository protocol, {@code false} otherwise.
* @see #newInstance(RepositorySystemSession, RemoteRepository)
* @since 2.1.0
*/
default boolean canHandle(String repositoryProtocol) {
return true;
}
```
##########
maven-resolver-transport-file/src/main/java/org/eclipse/aether/transport/file/FileTransporterFactory.java:
##########
@@ -65,6 +65,12 @@ public FileTransporterFactory setPriority(float priority) {
return this;
}
+
+ @Override
+ public boolean canHandle(String repositoryProtocol) {
Review Comment:
⚠️ **Stray blank line before `@Override`.** Minor style nit — the extra
blank line before the annotation is inconsistent with the rest of the codebase.
```suggestion
@Override
public boolean canHandle(String repositoryProtocol) {
return "bundle".equalsIgnoreCase(repositoryProtocol) ||
"file".equalsIgnoreCase(repositoryProtocol);
}
```
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/connector/transport/TransporterFactory.java:
##########
@@ -31,9 +33,39 @@
*/
public interface TransporterFactory {
+ /**
+ * A key for transporter properties.
+ * @see #getProperties()
+ */
+ public interface TransporterPropertyKey {}
+
+ /**
+ * Indicates whether this factory can handle the specified repository
protocol.
+ * Even if {@code true} is returned the factory may still refuse to create
a transporter for the given protocol.
+ *
+ * @param repositoryProtocol The repository protocol to check, may be
{@code null}.
+ * @return {@code true} if this factory can potentially handle the
specified repository protocol, {@code false} otherwise.
+ * @see #newInstance(RepositorySystemSession, RemoteRepository)
+ */
+ default boolean canHandle(String repositoryProtocol) {
+ return true;
+ }
+
+ /**
+ * Gets the properties exposing information about this transporter factory.
+ * Some are mandatory and some are transporter-specific.
+ *
+ * @return A map of transporter property keys with values, never {@code
null}.
+ * @throws UnsupportedOperationException if the transporter factory does
not implement this method.
+ */
+ default Map<TransporterPropertyKey, Object> getProperties() {
+ throw new UnsupportedOperationException("getProperties not
implemented");
Review Comment:
🔴 **Javadoc says `never {@code null}` but default throws
`UnsupportedOperationException`.** The contract is self-contradictory: the
Javadoc promises a non-null return, the `@throws` documents the exception, but
a caller following the `@return` contract has no reason to expect an exception.
As noted in the previous review, the idiomatic default for an optional SPI
method is a no-op return (`Collections.emptyMap()`), not a thrown exception. If
the method truly requires implementation, it should not be a `default` method —
make it abstract and force implementors to provide it.
```suggestion
default Map<TransporterPropertyKey, Object> getProperties() {
return Map.of();
}
```
--
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]
