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.

Sign up for GitHub

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

Jump to bottom

Relays+Infallible #2430

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Relays+Infallible #2430

wants to merge 3 commits into from
+3 −3

Conversation

mashe
Copy link

@mashe mashe commented Jun 18, 2022

freak4pc
freak4pc requested changes Aug 5, 2022
View reviewed changes
Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Lets get some tests on this and we can merge it in.
Thanks!

RxRelay/Relays+Infallible.swift Outdated
}
}

public extension PublishRelay {
Copy link
Member

Choose a reason for hiding this comment

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

Definitely needs some test coverage :)

Copy link
Member

Choose a reason for hiding this comment

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

(basically just something that compares the results of the relay to the infallible is "good enough" here).

@freak4pc freak4pc mentioned this pull request Aug 6, 2022
Closed
@freak4pc
Copy link
Member

freak4pc commented Aug 8, 2022

@mashe got the time to take this to the finishing line? 🙏

@rtharston
Copy link

This should just replace the relay specific parts of #2401.
Basically, just replace asInfallible(onErrorFallbackTo: .empty()) in the methods added in that PR with Infallible(self.asObservable())

@TTOzzi
Copy link
Contributor

TTOzzi commented Aug 11, 2022

It already has test cases in Infallible+Tests.swift 🤔

@mashe
Copy link
Author

mashe commented Aug 14, 2022

Thank you, everyone for keeping eye on the PR.
@freak4pc, I have followed @rtharston suggestion, so I reverted initial commit and updated existing .asInfallible() functions for Relays. I believe additional tests are not required, since @TTOzzi noted that those exist already.

@rtharston
Copy link

@freak4pc, any update on this?

@freak4pc freak4pc force-pushed the main branch 2 times, most recently from 1198683 to 5949cbd Compare April 21, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freak4pc freak4pc freak4pc requested changes

@volovely volovely volovely approved these changes

@TTOzzi TTOzzi TTOzzi approved these changes

@JamesNguyenSotatek JamesNguyenSotatek JamesNguyenSotatek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mashe @freak4pc @rtharston @TTOzzi @volovely @JamesNguyenSotatek