-
Notifications
You must be signed in to change notification settings - Fork 226
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
Fix a few nullability and other minor warnings #187
Conversation
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.
Can you make CI pass?
@@ -54,7 +55,7 @@ | |||
* @param view the view to scope for | |||
* @return a {@link LifecycleScopeProvider} against this view. | |||
*/ | |||
public static LifecycleScopeProvider from(View view) { | |||
public static LifecycleScopeProvider<ViewLifecycleEvent> from(@Nullable View view) { |
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.
This should not be @Nullable
. Also, let's make the return type <?>
to keep the ViewLifecycleEvent
an implementation detail
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.
So what triggered this was the explicit check for null -- can that be removed? I can just leave it alone with the warning too.
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.
We have jsr305 package annotations to signal to static analysis that parameters are nonnull by default
We have the nullcheck as a fallback because we don't trust external users' static analysis and fail eagerly :). Just leave the warning
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.
got it, clarified with a comment
@@ -36,7 +36,7 @@ | |||
return; | |||
} | |||
|
|||
Subscriber<? super T>[] newSubscribers = new Subscriber[subscribers.length]; | |||
@SuppressWarnings("unchecked") Subscriber<? super T>[] newSubscribers = new Subscriber[subscribers.length]; |
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.
Can this be made to not have generics issues?
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.
I don't think so, because you can't create an array of a generic type T. There's always going to be some type munging behind the scenes. It's either cast here, or cast somewhere else and either way it's unchecked.
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.
👍
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.
Please remove the comments and constant conditions suppression, they're not really necessary.
Can you fix the checkstyle errors? |
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.
Thanks!
Fix a few nullability and other minor warnings.