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

Implement 'toString' method for some Emitters #5995

Merged
merged 7 commits into from
May 6, 2018
Merged

Implement 'toString' method for some Emitters #5995

merged 7 commits into from
May 6, 2018

Conversation

strekha
Copy link
Contributor

@strekha strekha commented May 5, 2018

When use .create method it's unclear why emitter is null (if call toString or observe object via debugger).

@codecov
Copy link

codecov bot commented May 5, 2018

Codecov Report

Merging #5995 into 2.x will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #5995      +/-   ##
============================================
- Coverage     98.25%   98.24%   -0.02%     
+ Complexity     6051     6047       -4     
============================================
  Files           656      656              
  Lines         44078    44085       +7     
  Branches       6118     6118              
============================================
+ Hits          43311    43312       +1     
  Misses          231      231              
- Partials        536      542       +6
Impacted Files Coverage Δ Complexity Δ
...ctivex/internal/operators/single/SingleCreate.java 95.45% <100%> (+0.1%) 2 <0> (ø) ⬇️
...ex/internal/operators/flowable/FlowableCreate.java 97.41% <100%> (+0.01%) 6 <0> (ø) ⬇️
...nternal/operators/observable/ObservableCreate.java 99.14% <100%> (+0.88%) 2 <0> (ø) ⬇️
...eactivex/internal/operators/maybe/MaybeCreate.java 94.11% <100%> (+0.11%) 2 <0> (ø) ⬇️
...ernal/operators/completable/CompletableCreate.java 95.23% <100%> (+0.11%) 2 <0> (ø) ⬇️
...erators/flowable/FlowableOnBackpressureLatest.java 94.59% <0%> (-5.41%) 2% <0%> (ø)
...a/io/reactivex/internal/util/QueueDrainHelper.java 94.44% <0%> (-4.17%) 54% <0%> (-3%)
...l/operators/observable/ObservableFlatMapMaybe.java 90.84% <0%> (-3.93%) 2% <0%> (ø)
...tivex/internal/observers/FutureSingleObserver.java 94.33% <0%> (-3.78%) 24% <0%> (-1%)
...reactivex/internal/operators/single/SingleAmb.java 96.36% <0%> (-3.64%) 9% <0%> (-1%)
... and 22 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 90203b6...a75d712. Read the comment docs.

@akarnokd
Copy link
Member

akarnokd commented May 5, 2018

Please add unit tests that check these toString()s to some extent.

Copy link
Member

@akarnokd akarnokd left a comment

Choose a reason for hiding this comment

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

Please add unit tests.

Flowable.create(new FlowableOnSubscribe<Object>() {
@Override
public void subscribe(FlowableEmitter<Object> emitter) throws Exception {
assertTrue(emitter.toString().contains(entry.getValue().getSimpleName()));
Copy link
Member

Choose a reason for hiding this comment

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

If this fails, the test still passes because the error is turned into onError. Please add the .assertEmpty() after test(). In addition, please do the test for the serialized emitter as well.

@akarnokd
Copy link
Member

akarnokd commented May 5, 2018

The Maybe and Completable variants are missing.

Observable.create(new ObservableOnSubscribe<Object>() {
@Override
public void subscribe(ObservableEmitter<Object> emitter) throws Exception {
assertTrue(emitter.toString().contains(ObservableCreate.CreateEmitter.class.getSimpleName()));
Copy link
Member

Choose a reason for hiding this comment

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

Please add the serialized test here as well.

Copy link
Collaborator

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

Aren't there more cases where we should do this then?

I'm still unsure how much the benefit is worth it.

@akarnokd
Copy link
Member

akarnokd commented May 6, 2018

Aren't there more cases where we should do this then?

Maybe on the emitters of generators. Any user-facing API object could be a candidate.

I'm still unsure how much the benefit is worth it.

There is occasionally a question where the OP states the emitter is null and he is getting an NPE. Then I had to explain that when debugging, the toString value returns "null" and not the emitter reference is null.

@vanniktech
Copy link
Collaborator

Should we cover the generators too?

@strekha
Copy link
Contributor Author

strekha commented May 6, 2018

Think we should. So I will add this soon.
Are there any other places except the generators? Because I don't know the API so well.

@akarnokd
Copy link
Member

akarnokd commented May 6, 2018

The generate methods. Please put them into a separate PR.

Copy link
Collaborator

@vanniktech vanniktech left a comment

Choose a reason for hiding this comment

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

Alright. Separate PR it is.

@akarnokd akarnokd merged commit a957c78 into ReactiveX:2.x May 6, 2018
@strekha strekha deleted the tostring-for-emitters branch May 6, 2018 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants