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

fix many_select #217

Closed

Conversation

neilvyas
Copy link
Contributor

@neilvyas neilvyas commented Oct 28, 2017

It seems many_select is broken as far as satisfying the co-monad laws for co-bind / extend. I'm under the impression that many_select was intended to be co-bind/extend because it says so in the docstring and Bart de Smet mentions as much here.

The main issue is that first is not extract, as implemented. It should have type Observable[a] -> a but instead has type Observable[a] -> Observable[a]. Thus, this test fails because of extract. Note that first could be a lawful `extract, based on these laws (I briefly worked them out on paper).

Actually, the types are just all weird, because you can't go from Observable[a] -> a, because first isn't extract:

Observable[a] ->
(private: after calling map(ChainedObservable)) Observable[Observable[a]] ->
(after applying selector: Observable[a] -> Observable[b], which should have been Observable[a] -> b)
Observable[Observable[b]] (should have been Observable[b])

In fact, I don't think there's any method in rx with type Observable[a] -> b, since the only thing that looks like that is subscribe: Observer[a, b] -> Observable[a] -> "b" where b is the type of the side-effect and subscribe actually returns unit/void.

That said, a possible candidate for extract could be something like o.first().to_future(), which does have type Observable[a] -> b (or really Observable[Future[a]] -> Future[b], but whatever).

The second issue is that this implementation doesn't do anything interesting with extend / many_select. As far as I can tell (because the code is magic),

The third issue is that I think it's broken. The implementation, both here and in RxJS, is some serious magic. The tests in RxJS are the same (and thus also trivial), so I'm not convinced that this has ever worked. I gather that we're supposed to get something like:

>>> rx.Observable.from_iterable([1, 2, 3]).many_select(lambda x: x.first()). ...
... Observable([1, 2, 3], [2, 3], [3])

but instead we get `Observable([1], [2], [3]). I think RxJS is similarly broken.

Given these issues, I think some action items are

  1. stop trying to have co-monadic bind (also deprecate many_select as implemented, since it doesn't do anything), or fix many_select so it works like .NET.
  2. implement a functioning extract, probably as o.first().to_future()

I'm going to try to fix many_select, and just make the tests accommodate the fact that we don't have an extract.

@neilvyas
Copy link
Contributor Author

neilvyas commented Oct 28, 2017

apologies for the wall of text. I will be fixing many_select to work like .NET and adding more coverage. This should fix #218

@coveralls
Copy link

coveralls commented Oct 28, 2017

Coverage Status

Coverage increased (+0.02%) to 87.04% when pulling 577a51e on Affirm:neilvyas/fix-select-many-as-co-bind into 9437cf9 on ReactiveX:develop.

@neilvyas neilvyas changed the title what is up with many_select fix many_select Oct 28, 2017
@dbrattli
Copy link
Collaborator

@neilvyas A long time has gone, just wondering if you would be interested in updating this PR for the new pipe syntax?

@dbrattli dbrattli closed this May 5, 2019
@lock
Copy link

lock bot commented May 5, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants