Observable.flatten bugfix #1330

Merged
merged 1 commit into from Dec 28, 2015

Conversation

Projects
None yet
2 participants
@ForNeVeR
Contributor

ForNeVeR commented Dec 26, 2015

Current implementation of Observable.flatten never calls obs.OnComplete, and that leads to a bug in Paket.VisualStudio (and possibly adds problems for another users of this API).

Instead of writing that method here (it could be difficult for a general case), I've decided to simply use FSharp.Control.Reactive instead.

This PR will close fsprojects/Paket.VisualStudio#60.

Please note that you'll have to include FSharp.Control.Reactive.dll in a future builds of Paket.VisualStudio, and I've found that it's not trivial to achieve (because Paket.VisualStudio have no direct dependency on this package). In my local environment I got this fixed by adding reference to FSharp.Control.Reactive into a main Paket.VisualStudio assembly, but maybe there is some more conventional way.

Utils: use FSharp.Control.Reactive instead of manually redefining Obs…
…ervable.flatten.

This will fix a bug when observer.OnComplete never gets called in SearchPackagesByName operation.

@forki forki merged commit 4883fbf into fsprojects:master Dec 28, 2015

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 28, 2015

Member

thx for hunting down this bug. But the dependencies is very big, so I tried to fix it directly. Could you please review: afe98e4

thx

Member

forki commented Dec 28, 2015

thx for hunting down this bug. But the dependencies is very big, so I tried to fix it directly. Could you please review: afe98e4

thx

@ForNeVeR

This comment has been minimized.

Show comment
Hide comment
@ForNeVeR

ForNeVeR Dec 28, 2015

Contributor

@forki I got your point, I'll review (and test) that change tonight. Thanks for the answer.

Contributor

ForNeVeR commented Dec 28, 2015

@forki I got your point, I'll review (and test) that change tonight. Thanks for the answer.

@ForNeVeR

This comment has been minimized.

Show comment
Hide comment
@ForNeVeR

ForNeVeR Dec 28, 2015

Contributor

@forki yup that works very good (I've tried the latest releases Paket.VisualStudio version), and the code looks ok to me. Thank you!

Contributor

ForNeVeR commented Dec 28, 2015

@forki yup that works very good (I've tried the latest releases Paket.VisualStudio version), and the code looks ok to me. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment