-
Notifications
You must be signed in to change notification settings - Fork 429
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
Concat error #564
Concat error #564
Conversation
Added a convenience method for concatenating an error to a SignalProducer.
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 seems sensible 👍
Can you add another test for concatenating an error to a failing producer? (Seems silly, but just to verify that obviously the original error is the one emitted).
@NachoSoto good test suggestion! Will add that shortly. |
Added an additional test case to verify that the error of the original signal is emitted - this ensures that concatenated error doesn't override or silence the original error in some way.
expect(results[3].error) == .default | ||
} | ||
|
||
it("should emit own error when present") { |
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'm not quite sure of the wording of this expectation. Thoughts?
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.
"should not concat the error", perhaps?
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 feel like that contradicts the earlier expectation - unless the reader looks at the implementation. However in my usage of Quick, I try to write expectations that could stand-alone without the implementation and still make sense.
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.
“should not concat a subsequent error if the earlier producer has failed”?
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.
"should not emit concatenated error to a failed producer"?
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.
Altered wording of expectation to clarify it's behaviour.
Given that
SignalProducer
already supports concatenating a value or a signal, being able to concatenate an error seemed logical. I personally found this useful when implementing a signal that defaulted to an error if nothing matched a filter. For example:Checklist