search

fsprojects / FSharp.Control.Reactive

Extensions and wrappers for using Reactive Extensions (Rx) with F#.
http://fsprojects.github.io/FSharp.Control.Reactive
Other
284 stars 58 forks source link

combineLatest and replay improvements #39

Closed marklam closed 9 years ago

marklam commented 9 years ago

Make combineLatest variants more like zip, including changing combineLatest to take two IObservables. Fix replay variants to call the correct underlying functions, add replay...On to take schedulers, especially useful when testing.

panesofglass commented 9 years ago

Thank you! This looks nice! The only problem I see is that this should require a major version number change b/c we are breaking the signatures of existing functions. Would you consider restoring the original function names and using new names for the new functions?

marklam commented 9 years ago

The name changes I've made are: combineLatest --> combineLatestSeq combineLatestMap --> combineLatestSeqMap and a new combineLatest with a different signature, taking two IObservables. This matches the zip, zipSeq and zipSeqMap variants.

If we leave the original names for combineLatest and combineLatestMap, and name the two-observable version combineLatest2 (or whatever) the names won't correspond with the zip functions.

I'm just looking at some of the functions I haven't yet made use of, and I think the signatures could also be improved for: collectMerge, delayUnitl (sic), equalsCompare, equalsSeqComparer, repeatCount (maybe - that's a particularly risky change) and throttleComputed Maybe we could consider making changes to (some of) these all together and that might be worth the version bump?

panesofglass commented 9 years ago

Yes, if we make the effort to clean these up, I would agree it's worth the version bump. Do you mind making the changes?

panesofglass commented 9 years ago

@marklam, thank you for your help and attention to detail!

marklam commented 9 years ago

No problem, hope these are OK. I put the change to repeatCount in a separate commit, because it's the only one that had potential to change behaviour but still compile - in the case of creating an Observable<int>, code to generate three 7's would now generate seven 3's. I'm not sure how likely that is in the wild, but it's at least worth drawing attention to.

marklam commented 9 years ago

Just remembered I wanted to add a scheduler-capable version of timestamp too :-)

panesofglass commented 9 years ago

@marklam, thanks! I'll take a look soon.

marklam commented 9 years ago

Added issues #47 and #48 to cover (most of) the changes in this PR.