-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Please add unit tests that check these |
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 add unit tests.
Flowable.create(new FlowableOnSubscribe<Object>() { | ||
@Override | ||
public void subscribe(FlowableEmitter<Object> emitter) throws Exception { | ||
assertTrue(emitter.toString().contains(entry.getValue().getSimpleName())); |
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.
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.
The |
Observable.create(new ObservableOnSubscribe<Object>() { | ||
@Override | ||
public void subscribe(ObservableEmitter<Object> emitter) throws Exception { | ||
assertTrue(emitter.toString().contains(ObservableCreate.CreateEmitter.class.getSimpleName())); |
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 add the serialized test here as well.
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.
Aren't there more cases where we should do this then?
I'm still unsure how much the benefit is worth it.
Maybe on the emitters of generators. Any user-facing API object could be a candidate.
There is occasionally a question where the OP states the emitter is null and he is getting an |
Should we cover the generators too? |
Think we should. So I will add this soon. |
The |
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.
Alright. Separate PR it is.
When use
.create
method it's unclear whyemitter
is null (if calltoString
or observe object via debugger).