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

Remove @NonNull annotations in BiConsumer method parameter #5257

Merged
merged 1 commit into from
Apr 2, 2017

Conversation

bloderxd
Copy link
Contributor

@bloderxd bloderxd commented Apr 1, 2017

This PR is part of #5216

This PR remove @nonnull annotations from BiConsumer method parameters, there's no sense use these annotations in Single context because in this case BiConsumer always brings you a null parameter.

@codecov
Copy link

codecov bot commented Apr 1, 2017

Codecov Report

Merging #5257 into 2.x will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5257      +/-   ##
============================================
- Coverage     95.97%   95.94%   -0.03%     
+ Complexity     5751     5743       -8     
============================================
  Files           628      628              
  Lines         41077    41077              
  Branches       5699     5699              
============================================
- Hits          39422    39412      -10     
- Misses          664      668       +4     
- Partials        991      997       +6
Impacted Files Coverage Δ Complexity Δ
...al/operators/observable/ObservableSampleTimed.java 90% <0%> (-8.34%) 3% <0%> (ø)
...ternal/operators/flowable/FlowableSubscribeOn.java 91.52% <0%> (-6.78%) 2% <0%> (ø)
...ternal/operators/flowable/FlowableSampleTimed.java 89.7% <0%> (-5.89%) 3% <0%> (ø)
...l/operators/observable/ObservableFlatMapMaybe.java 88.88% <0%> (-5.23%) 2% <0%> (ø)
...rnal/subscribers/SinglePostCompleteSubscriber.java 94.87% <0%> (-5.13%) 14% <0%> (-1%)
...ternal/operators/completable/CompletableUsing.java 95.23% <0%> (-4.77%) 4% <0%> (ø)
...internal/operators/completable/CompletableAmb.java 94.91% <0%> (-3.39%) 10% <0%> (-1%)
...activex/internal/operators/single/SingleCache.java 97.05% <0%> (-2.95%) 23% <0%> (-1%)
...activex/internal/operators/single/SingleUsing.java 97.59% <0%> (-2.41%) 4% <0%> (ø)
...va/io/reactivex/internal/queue/SpscArrayQueue.java 97.61% <0%> (-2.39%) 22% <0%> (-1%)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa58d36...d9a2e60. Read the comment docs.

@akarnokd
Copy link
Member

akarnokd commented Apr 1, 2017

/cc @vanniktech

I belive BiConsumer is very often appears with zip and combineLatest where the @NonNull annotation is important.

@JakeWharton
Copy link
Contributor

It's not that important. It just means nullability contracts need to be defined where BiConsumer is used instead of on the interface itself.

@bloderxd
Copy link
Contributor Author

bloderxd commented Apr 1, 2017

Well, it's not in all context that @nonnull annotation is correct in BiConsumer, then I guess it's not safe let it there

@vanniktech
Copy link
Collaborator

Might make more sense to delete the annotations since the interface is used in different places where different parameters are sometimes null and sometimes not.

Having different interfaces with different annotations tailored for each use case might be the best option although it wouldn't be backwards compatible at least not source and binary compatible.

@bloderxd
Copy link
Contributor Author

bloderxd commented Apr 1, 2017

@vanniktech like this example?

 public void biconsumerWithCorrectNullableAnnotationInThrowable() {
        Single.just(1).subscribe(new SingleBiConsumer<Integer, Throwable>() {
            @Override
            public void accept(@Nullable Integer integer, @Nullable Throwable throwable) throws Exception {
                assertNull(throwable);
            }
        });
    }

Create a extesion of BiConsumer only for Single Context?

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.

4 participants