Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11293132:
Add a Futures.forEach function for asynchronously iterating over a collection. (Closed)
|
Created:
8 years, 1 month ago by nweiz Modified:
8 years ago CC:
reviews_dartlang.org, Bob Nystrom Visibility:
Public. |
DescriptionAdd a Futures.forEach function for asynchronously iterating over a collection.
Committed: https://code.google.com/p/dart/source/detail?r=15473
Patch Set 1 #
Total comments: 11
Patch Set 2 : Code review changes #
Total comments: 6
Patch Set 3 : Code review change #
Messages
Total messages: 11 (0 generated)
+floitsch lgtm, just minor nits on my part. Adding Florian to keep him in the loop and make sure its ok to do these non-breaking changes in bleeding_edge (as opposed to lib_v2) https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:263: /// Runs [fn] for each element in [input] in order, moving to the next element nit: use /** */ docs (for consistency) https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:267: /// The return values of all [Future]s are discarded. Any errors will cause the nit 80 col https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:268: /// iteration to stop and will be piped through the return value. to avoid confusion with the returned values above, maybe rephrase as 'through the returned future'? https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:269: static Future forEach(Iterable input, Future fn(element)) { nit: rename fn as f? (trying to match the signature used in collection.forEach
One suggestion, but LGTM either way. Please get the OK from Florian, though. https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:267: /// The return values of all [Future]s are discarded. Any errors will cause the Pub doesn't need a return value, but to be more generally useful, it would be good to not drop it. How about making this complete with the list of values? Then, given that, how about calling this map() instead?
https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:263: /// Runs [fn] for each element in [input] in order, moving to the next element On 2012/11/06 23:23:51, Siggi Cherem (dart-lang) wrote: > nit: use /** */ docs (for consistency) Done. https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:267: /// The return values of all [Future]s are discarded. Any errors will cause the On 2012/11/06 23:23:51, Siggi Cherem (dart-lang) wrote: > nit 80 col Done. https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:267: /// The return values of all [Future]s are discarded. Any errors will cause the On 2012/11/07 01:09:43, Bob Nystrom wrote: > Pub doesn't need a return value, but to be more generally useful, it would be > good to not drop it. How about making this complete with the list of values? > > Then, given that, how about calling this map() instead? I liked this idea when you first proposed it, but upon thinking about it more I don't think it's the right way to go. This method is explicitly procedural and side-effect based, whereas mapping is much more functional. I don't think it makes sense to conflate the two; I think the situations where the order of execution of the futures matters is going to be largely disjoint from the situations where the return values matter. https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:268: /// iteration to stop and will be piped through the return value. On 2012/11/06 23:23:51, Siggi Cherem (dart-lang) wrote: > to avoid confusion with the returned values above, maybe rephrase as 'through > the returned future'? Done. https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:269: static Future forEach(Iterable input, Future fn(element)) { On 2012/11/06 23:23:51, Siggi Cherem (dart-lang) wrote: > nit: rename fn as f? (trying to match the signature used in collection.forEach Done.
https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/1/sdk/lib/core/future.dart#newc... sdk/lib/core/future.dart:267: /// The return values of all [Future]s are discarded. Any errors will cause the On 2012/11/07 20:50:07, nweiz wrote: > On 2012/11/07 01:09:43, Bob Nystrom wrote: > > Pub doesn't need a return value, but to be more generally useful, it would be > > good to not drop it. How about making this complete with the list of values? > > > > Then, given that, how about calling this map() instead? > > I liked this idea when you first proposed it, but upon thinking about it more I > don't think it's the right way to go. This method is explicitly procedural and > side-effect based, whereas mapping is much more functional. I don't think it > makes sense to conflate the two; I think the situations where the order of > execution of the futures matters is going to be largely disjoint from the > situations where the return values matter. SGTM.
Sorry it took so long. I would prefer a more fluent solution. https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart#n... sdk/lib/core/future.dart:271: static Future forEach(Iterable input, Future f(element)) { I don't think we need/want this function. We should rather write: list.mappedBy(f).foldX(Future.wait) foldX (still looking for a name) would take the first value from the iterable and then combine it with the remaining elements. Future.wait (still looking for a name) would take two futures and produce a future when both are finished. Now this is not yet perfect (empty lists...), and even when writing this comment I have already better ideas of doing this, but the general idea is there.
https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart#n... sdk/lib/core/future.dart:271: static Future forEach(Iterable input, Future f(element)) { On 2012/11/19 10:05:42, floitsch wrote: > I don't think we need/want this function. We should rather write: > list.mappedBy(f).foldX(Future.wait) > > foldX (still looking for a name) How about reduce() for the operation that takes a seed and fold() for the one that uses the first element as the seed?
https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart#n... sdk/lib/core/future.dart:271: static Future forEach(Iterable input, Future f(element)) { On 2012/11/19 10:05:42, floitsch wrote: > I don't think we need/want this function. We should rather write: > list.mappedBy(f).foldX(Future.wait) > > foldX (still looking for a name) would take the first value from the iterable > and then combine it with the remaining elements. > Future.wait (still looking for a name) would take two futures and produce a > future when both are finished. > > Now this is not yet perfect (empty lists...), and even when writing this comment > I have already better ideas of doing this, but the general idea is there. That wouldn't do the same thing as this function. It would start running each future immediately in parallel, rather than running them in sequence. To do this function, you'd need to do something like: list.foldX((element, future) { return future.chain((_) => f(element)); }, new Future.immediate()); I think that's complicated enough that it's worth adding a helper function. https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart#n... sdk/lib/core/future.dart:271: static Future forEach(Iterable input, Future f(element)) { On 2012/11/19 19:55:07, Bob Nystrom wrote: > On 2012/11/19 10:05:42, floitsch wrote: > > I don't think we need/want this function. We should rather write: > > list.mappedBy(f).foldX(Future.wait) > > > > foldX (still looking for a name) > > How about reduce() for the operation that takes a seed and fold() for the one > that uses the first element as the seed? In terms of naming, I'd rather see the operation have the same name for both taking a seed and not taking a seed. I think reduce() and fold() are both reasonable names; they're both common in other languages. This is somewhat off-topic, but I don't like how much longer "mappedBy" is than "map". I'm not sure the extra length is worth having it be adjective-form, especially if there are other method names that are verb-form like "fold".
https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart#n... sdk/lib/core/future.dart:264: * Runs [fn] for each element in [input] in order, moving to the next element Should this be "[f]"?
https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart File sdk/lib/core/future.dart (right): https://codereview.chromium.org/11293132/diff/5001/sdk/lib/core/future.dart#n... sdk/lib/core/future.dart:264: * Runs [fn] for each element in [input] in order, moving to the next element On 2012/11/19 21:32:59, Andrei Mouravski wrote: > Should this be "[f]"? Yes. Done.
LGTM. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
