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.

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

Extended Sample operation to support a default value #1457

Merged
merged 10 commits into from
Jan 9, 2020

Conversation

aspitz
Copy link
Contributor

@aspitz aspitz commented Oct 14, 2017

Added the ability to set a default value for the sampler to return in the event that there were no new elements between sampler ticks

@RxPullRequestBot
Copy link

RxPullRequestBot commented Oct 14, 2017

1 Warning
⚠️ No CHANGELOG changes made

Generated by 🚫 Danger

@kzaher
Copy link
Member

kzaher commented Oct 18, 2017

Let me think about this suggestion.

@freak4pc
Copy link
Member

This is kinda interesting actually. Since this is purely additive there’s no real issue with adding this.

The only issue I’m seeing here is that it’s diverging from ReactiveX and from the original usage of sample (turning into something like an interval withLatestFrom).

Still could be an interesting addition 🤔🤔🤔 hard to say

@aspitz-intrepid
Copy link

@kzaher any further thoughts?

@freak4pc
Copy link
Member

I still find this to be a good addition. More specifically, it would be quite hard accomplishing this behavior by combining other operators in RxSwift.

@aspitz any chance you'd be willing to bring this up-to-date to the latest standards or the repo? Otherwise, I can find someone else to take over this code.

@aspitz-intrepid
Copy link

I can fix this up :)

@freak4pc
Copy link
Member

freak4pc commented Jan 2, 2020

Awesome :) @aspitz-intrepid

Ayal Spitz and others added 3 commits January 2, 2020 23:12
… the event that there were no new elements between sampler ticks
… the event that there were no new elements between sampler ticks
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.

Looks good, thank you! Just a few quick notes.

RxSwift/Observables/Sample.swift Outdated Show resolved Hide resolved
RxSwift/Observables/Sample.swift Outdated Show resolved Hide resolved
RxSwift/Observables/Sample.swift Outdated Show resolved Hide resolved
RxSwift/Observables/Sample.swift Outdated Show resolved Hide resolved
RxSwift/Observables/Sample.swift Show resolved Hide resolved
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.

LGTM! Final nitpick and we can merge this. Thank you :)

RxSwift/Observables/Sample.swift Outdated Show resolved Hide resolved
@freak4pc
Copy link
Member

freak4pc commented Jan 9, 2020

Thank you ! @aspitz

@freak4pc freak4pc merged commit dc986a8 into ReactiveX:develop Jan 9, 2020
gringoireDM pushed a commit to gringoireDM/RxSwift that referenced this pull request Mar 11, 2020
BenNunOhad pushed a commit to BenNunOhad/RxSwift that referenced this pull request Jul 12, 2023
GalBerezansky pushed a commit to GalBerezansky/RxSwift that referenced this pull request Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants