-
Notifications
You must be signed in to change notification settings - Fork 144
Expose transporter details per protocol #1771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
cd09367 to
9079c94
Compare
| * @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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to ease listing all transporters it would be better to expose a list of supported protocols (potentially including a wildcard). WDYT @cstamas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't requesting an instance for RemoteRepository (and getting NoTransportEx) same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially as sometimes you cannot tell ahead of time. for HTTP yes, but for file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is different in the following aspect: This just requires a protocol not a full blown remote repository. It doesn't rely on exceptions but just returns a boolean.
|
I am for exposing (what is related to that in this PR), but I am -1 on See https://github.com/apache/maven-resolver/blob/master/maven-resolver-api/src/test/java/org/eclipse/aether/repository/RemoteRepositoryTest.java So |
|
We have cases like |
|
I looked at all transporters we have, and while it is true that some evaluate more than just protocol, they all evaluate the protocol first. Therefore having this aspect separated is IMHO useful for this feature (but also for determining the correct transporter) as in the best case you don't have the overhead of throwing an exception as you bail out for a non-matching protocol first. |
Instead of purely relying on exception handling when a transporter cannot deal with a certain kind of repository an in advance check on the repository protocol is introduced. This closes #1770
9079c94 to
6233e73
Compare
|
https://github.com/taxilang/taxilang/blob/7d83962001b73a37b5c18f8992ef9c6e87d17e97/package-manager/src/main/java/org/taxilang/packagemanager/transports/TaxiFileSystemTransport.kt#L25 |
|
Also, some transport may support or not support something based on config... and again, protocol is just not enough to decide all this. Either use repo url or whole repo instance. Hence, my 5 cents would be:
|
|
I am open for other suggestions :-). I am not saying that this covers all transporters, but this is just an additional layer. If there is a decision based on protocol impossible the transporter just returns |
Instead of purely relying on exception handling when a transporter cannot deal with a certain kind of repository an in advance check on the repository protocol is introduced
This closes #1770
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.