Add rx-merge-delay-error operator#417
Conversation
|
|
kirkshoop
left a comment
kirkshoop
left a comment
There was a problem hiding this comment.
Looking great! just a few more changes I think.
| [state](std::exception_ptr e) { | ||
| if(--state->pendingCompletions == 0) { | ||
| state->out.on_error(e); | ||
| } else { |
There was a problem hiding this comment.
perhaps when the last stream terminates in an error the error should be added to the composite_exception and then the composite_exception is sent to out.on_error?
| [state](std::exception_ptr e) { | ||
| if(--state->pendingCompletions == 0) { | ||
| state->out.on_error(e); | ||
| } else { |
| #else | ||
| #define NOEXCEPT | ||
| #endif | ||
|
|
There was a problem hiding this comment.
this should be in rx-includes.hpp and should be specific to MSVC version (I know that recent versions support noexcept) and should have an unambiguous name to prevent conflicts RXCPP_NOEXCEPT
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
it is not required to keep the [perf] tagged tests above, since the error handling perf is not being tested.
| }); | ||
| auto actual = res.get_observer().messages(); | ||
| REQUIRE(required == actual); | ||
| } |
There was a problem hiding this comment.
it would be nice to be able to check that the exception list had the correct exceptions stored.. but I think that would require a lot of work in the test infrastructure. something for the future.
There was a problem hiding this comment.
yes, I agree - I also spotted that the test does not cover the actual exception assertation. I'll take a look at that later
| \sample | ||
| \snippet merge_delay_error.cpp threaded merge sample | ||
| \snippet output.txt threaded merge sample | ||
| */ |
There was a problem hiding this comment.
These examples need to be defined in a new file next to https://github.com/Reactive-Extensions/RxCpp/blob/master/Rx/v2/examples/doxygen/merge.cpp
You might want to have an example that demonstrates the composite_error
There was a problem hiding this comment.
I added those doxygen examples:
- https://github.com/nitrram/RxCpp/blob/merge_error_delay/Rx/v2/examples/doxygen/merge_delay_error.cpp
- https://github.com/nitrram/RxCpp/blob/merge_error_delay/Rx/v2/examples/doxygen/composite_exception.cpp
-- hopefully sufficiently enough
Yet if there is something missing, let me know.
There was a problem hiding this comment.
looks great! I did not see these added to https://github.com/Reactive-Extensions/RxCpp/blob/master/projects/doxygen/CMakeLists.txt
did I miss that?
There was a problem hiding this comment.
Ah, my bad! I forgot that. I should also change the documentation at rx-merge_delay_error. cpp so that it matches the tests which I'm afraid does not at the moment. Unfortunately I'll be at a computer not sooner than on Sunday. But I'll fix it right then. Thanks for the review and patience
There was a problem hiding this comment.
Ok, so eventually I managed that. I added those 2 samples among the others. The documentation turned to be fine to me after all. When it's approved, shall I do something about it (squash commits...), before it gets merged?
Added RXCPP_NOEXCEPT macro; Added doxygen scenarios for composite_exception and merge_delay_error; Fixed composing exception in merge_delay_error operator; Modified test for merge_delay_operator
|
Thank you so much for adding the |
3 participants
The operator and its test is copy-pasted and modified accordingly from
mergeIt should behave the same way as JavaRx
mergeDelayError()in the terms of semantics