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

whenAll: always yields [RACUnit defaultUnit] #43

Closed
bvanderveen opened this issue Aug 27, 2012 · 11 comments
Closed

whenAll: always yields [RACUnit defaultUnit] #43

bvanderveen opened this issue Aug 27, 2012 · 11 comments

Comments

@bvanderveen
Copy link
Contributor

The implementation of whenAll::

+ (RACSubscribable *)whenAll:(NSArray *)subscribables {
    return [self combineLatest:subscribables reduce:^(RACTuple *xs) { return [RACUnit defaultUnit]; }];
}

The call to sendNext: within combineLatest:reduce:

[subscriber sendNext:reduceBlock([RACTuple tupleWithObjectsFromArray:orderedValues])];

The result is that subscribers to the subscribable returned by whenAll: always receive [RACUnit defaultUnit]. Unless I'm missing something obvious.

A possible fix would be:

+ (RACSubscribable *)whenAll:(NSArray *)subscribables {
    return [self combineLatest:subscribables reduce:^(RACTuple *xs) { return xs; }];
}

Thoughts?

@jspahrsummers
Copy link
Member

This is what the method is documented to do. Are you saying that you would prefer a different interface? If so, what would be the motivation?

@bvanderveen
Copy link
Contributor Author

Currently, the subscribables passed to whenAll: have to side-effects to capture their values (if any). It would be nice if the value yielded by whenAll: was an ordered tuple of the values of the subscribables from which it was created, rather than a unit value.

@bvanderveen
Copy link
Contributor Author

Ah yes, missed that documentation. :S

I would prefer the return value be a tuple so I don't have to introduce side-effects (e.g., doNext:) to retrieve those values. I could achieve the same result with combineLatest:reduce: (where the reduceBlock is a simple identity, as in the change I proposed) but it might be nice to have a shorthand for that.

@bvanderveen
Copy link
Contributor Author

@joshaber
Copy link
Member

-whenAll: in its current form is pretty minimally useful. The name also seems odd to me. Feels more like -whenAny: than -whenAll:. Lol @ Pastjosh. So what if we do this. Deprecate -whenAll:, add -whenAny: which passes the tuple from combineLatest:reduce: through.

@jspahrsummers
Copy link
Member

I'm 👍 on making the changes discussed in this thread. I think it makes the most sense to just add a combineLatest: (with no reduce block) method, though. That gives you API consistency, and has a more transformative (rather than effective) feel.

@joshaber
Copy link
Member

@jspahrsummers I almost suggested that but it felt weird to call it -combineLatest: when it wasn't actually, you know, combining (well, I guess it does into a tuple but whatev).

@jspahrsummers
Copy link
Member

Right, it combines into a tuple. I don't know why that's any less of a combination. :P

@bvanderveen
Copy link
Contributor Author

👍 for adding -combineLatest: which has the same behavior as -whenAll: except that the tuple of values is returned.

@bvanderveen
Copy link
Contributor Author

Perhaps +combineLatest:reduce: could be deprecated in favor of composing +combineLatest: with a -select:?

@jspahrsummers
Copy link
Member

Fixed by #44.

andersio pushed a commit that referenced this issue Sep 22, 2016
Remove workarounds for compiler warning about StaticString
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants