Skip to content
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

no overrides, and null types #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

travisfw
Copy link

@travisfw travisfw commented Aug 8, 2020

With parameter defaulting to null, overrides aren't needed, neither of the inline fun nor of the constructor. Also, since weak references could always return null, their generic type should always be nullable.

With parameter defaulting to null, overrides aren't needed, neither of the inline fun nor of the constructor. Also, since weak references could always return null, their generic type should always be nullable.
Copy link
Contributor

@pedrovgs pedrovgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @travisfw thanks for your PR. I'm checking the code and I don't get why would you want to create a weak reference with a null value. Could you please describe why you think this change is interesting for the project?

@travisfw
Copy link
Author

It's a minor difference, but if the type the reference wraps is nullable (which it always should be because weak references could always return null), then, logically, there is no need for the reference instance itself to be nullable. Having a nullable reference wrap a nullable type is somewhat redundant, and necessitates a null check. It seems more canonical to avoid nullability wherever possible.
This isn't something I would argue about; just answering the question.

Copy link
Contributor

@pedrovgs pedrovgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading your answer @travisfw I think we should not mix the behavior of the null reference with the public API. If I'm not wrong, the current implementation of this API is designed to avoid the usage of null values. This makes sense because even when a weak reference can return a null value, when creating the weak reference instance it makes no sense to me to initialize it with a null value. At least, it makes no sense to me. That's why the public API does not accept null values but it can return a null instance.

I do not think this is a change we want to add to the project. However, I'm requesting the review of other team members just to ensure I'm not missing something. @Karumi/commanders could some of you take a look?

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.

2 participants