maven-resolver icon indicating copy to clipboard operation
maven-resolver copied to clipboard

Null implementations shall return null

Open michael-o opened this issue 4 years ago • 3 comments
trafficstars

@cstamas This breaks stuff and should be in 2.x only. The motivation is that if the class name says Null... it shall return null and not be a Default... impl. WDYT?

michael-o avatar Jun 20 '21 10:06 michael-o

Null impl does not mean that a method returns null. It means that the class and method does nothing - no behavior.

Dňa ne 20. 6. 2021, 12:55 Michael Osipov @.***> napísal(a):

@cstamas https://github.com/cstamas This breaks stuff and should be in 2.x only. The motivation is that if the class name says Null... it shall return null and not be a Default... impl. WDYT?

You can view, comment on, or merge this pull request online at:

https://github.com/apache/maven-resolver/pull/113 Commit Summary

  • Null implementations shall return null

File Changes

Patch Links:

  • https://github.com/apache/maven-resolver/pull/113.patch
  • https://github.com/apache/maven-resolver/pull/113.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apache/maven-resolver/pull/113, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH7ER7CPXFIWKXFG3DSWH3TTXCLLANCNFSM4676ZRHA .

Tibor17 avatar Jun 20 '21 16:06 Tibor17

Null impl does not mean that a method returns null. It means that the class and method does nothing - no behavior.

I don't understand that. How can be no behaivor if it invokes a getter?

michael-o avatar Jun 20 '21 17:06 michael-o

Every Null...Class means no behavior. The generál terminology. Also nongetter methods.

Dňa ne 20. 6. 2021, 19:10 Michael Osipov @.***> napísal(a):

Null impl does not mean that a method returns null. It means that the class and method does nothing - no behavior.

I don't understand that. How can be no behaivor if it invokes a getter?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apache/maven-resolver/pull/113#issuecomment-864584498, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH7ER4JTKJIEEPV3UEDOEDTTYOJLANCNFSM4676ZRHA .

Tibor17 avatar Jun 21 '21 18:06 Tibor17

@cstamas What is your opinion, does this make sense?

michael-o avatar Jun 04 '24 17:06 michael-o

I think the classes are named badly. These "default" (and not "null") selectors are meant to not interfere with already equipped repositories.

In short, you know that when you have a RemoteRepository instance created with RemoteRepository.Builder, it is "bare", in a sense, it carries ONLY things set by builder: id, type, url (and policies but that is irrelevant). There is NO AUTH/PROXY, anything "networking" related set on it. To "equip" RR instance with these, one must call one of these (use case dependant):

  • https://maven.apache.org/resolver/apidocs/org/eclipse/aether/RepositorySystem.html#newDeploymentRepository(org.eclipse.aether.RepositorySystemSession,org.eclipse.aether.repository.RemoteRepository)
  • https://maven.apache.org/resolver/apidocs/org/eclipse/aether/RepositorySystem.html#newResolutionRepositories(org.eclipse.aether.RepositorySystemSession,java.util.List)

And this method will return "equipped" RR instance (Moreover, when this happens within Maven, how Resolver is integrated with Maven, this means it will pick up things like auth and proxies etc from Maven settings.xml and related stuff).

Now, these (default) instances are "null" in a way, they do not obtain any info from anywhere, but from repository itself: if you have a "bare" RR, it will end up with "null", but IF you have an "equipped" RR instance, it will return what equipped RR has.

Hence, IMHO the names of these classes are bad, they are not "null" but rather "passthrough" or alike.

cstamas avatar Jun 05 '24 08:06 cstamas

Would you rename them for 2.0.0? That feels natural...

michael-o avatar Jun 05 '24 13:06 michael-o

Created https://issues.apache.org/jira/browse/MRESOLVER-565

cstamas avatar Jun 05 '24 13:06 cstamas

Superseded by https://github.com/apache/maven-resolver/pull/503

cstamas avatar Jun 13 '24 11:06 cstamas