RxTest: Adding XCTAssertEqual methods for Void streams#2485
Closed
rkreutz wants to merge 1 commit intoReactiveX:mainfrom
Closed
RxTest: Adding XCTAssertEqual methods for Void streams#2485rkreutz wants to merge 1 commit intoReactiveX:mainfrom
rkreutz wants to merge 1 commit intoReactiveX:mainfrom
Conversation
1198683 to
5949cbd
Compare
Contributor
nikolaykasyanov
left a comment
There was a problem hiding this comment.
That would be nice to have, anything blocking the merge here?
Member
|
Cleaning up the repo and closing old PRs. If this is still something you're interested in pursuing, feel free to comment and we can re-open and discuss. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This addresses #1552.
It basically treats
nextevents on streams withVoidas element as if they were the same.I am aware there was another PR addressing this which got closed, however I do think it is worth raising the discussion again and updating the PR to include
MaybeandSinglestreams as well.The reasons I believe it is worth revisiting the original stance on this is twofold:
Voidas if they wereEquatable, e.g:Equatableconformance toVoid, however that will probably be limited to whatever Swift version that is introduced and won't be backwards compatible so we would lock-out library consumers of this nicer API until they are able to update to the latest Swift version. If we go ahead with merging this PR, we can then add#if swift()macros around theseVoidstream implementations so they do not clash with theEquatableimplementations.With that said, it might not be much of an issue to most people that use this library, but that might be due to the fact that tests are usually neglected (don't usually follow the same strict guidelines as "production" code, or worst, not implemented at all), at least that has been my previous experience, so any opportunity to make testing a bit easier might contribute to a future with more reliable software (one can hope 😂).
Edit: I also noticed there was a concern around tuples (like what to do in the case of
(Void, Void)elements or even(Int, Void)or any other combination), should we keep adding methods to support those cases? Though I believe this is unrealistic, I'd say 99% of the time people would useVoidstreams rather than(Void, Void)and tuples with different types (like(Int, Void)) is even less likely since what most would do would be to just map to the actual value needed (Intin this case). So I don't think it would be an issue not providing this simpler interface to tuples withVoidelements.