Skip to content

Conversation

@michael-o
Copy link
Member

@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?

@Tibor17
Copy link
Contributor

Tibor17 commented Jun 20, 2021 via email

@michael-o
Copy link
Member Author

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?

@Tibor17
Copy link
Contributor

Tibor17 commented Jun 21, 2021 via email

@michael-o
Copy link
Member Author

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

@cstamas
Copy link
Member

cstamas commented Jun 5, 2024

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):

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.

@michael-o
Copy link
Member Author

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

@cstamas
Copy link
Member

cstamas commented Jun 5, 2024

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

@cstamas
Copy link
Member

cstamas commented Jun 13, 2024

Superseded by #503

@cstamas cstamas closed this Jun 13, 2024
@cstamas cstamas deleted the incorrect-null-impl branch June 13, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants