Bug #84189 for future: Behavior of repeat {...} foreach => [] may be counter-intuitive

Mon Mar 25 01:12:20 2013 TOSHIOITO [...] cpan.org - Ticket created

Subject:

Behavior of repeat {...} foreach => [] may be counter-intuitive

When Future::Utils::repeat() function is called with foreach => [] option, the CODE block is executed once with undef value given as the $item. Because this behavior may be counter-intuitive for users, it should at least be documented. Example: use Future; use Future::Utils qw(repeat); use Data::Dumper; repeat { my $item = shift; print Dumper $item; return Future->new->done($item); } foreach => []; Result: $VAR1 = undef;

Mon Mar 25 16:58:53 2013 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Mon Mar 25 01:12:20 2013, TOSHIOITO wrote: Show quoted text

> When Future::Utils::repeat() function is called with foreach => [] > option, the CODE block is executed once with undef value given as the > $item. > > Because this behavior may be counter-intuitive for users, it should at > least be documented.

Hmmyes, I hadn't thought that one all the way through before. It is a little awkward. It's awkward mostly because the type of Future returned by the first iteration of the repeat {} block is used as a prototype for the Future returned by the repeat itself. If there was no return, we'd have nothing to return from the overall thing. At this point I can think of three possible behaviours, but none of them jump out to me as overly appealing: 1) (immediately) throw an exception complaining about iteration on an empty array 2) return an immediately-failed Future complaining about this 3) return an immediately-done Future with no contents The trouble with 1 is that it sortof upsets the "failures turn into Failed futures, not immediate exceptions" idea; and the trouble with 2 and 3 is that it requires repeat() to invent a Future, which means it won't respect subclassing. I wonder instead whether we could benefit from a foreach/else style notation: repeat { ... } foreach => \@array, else => sub { Future->new->fail("None") }; but then that doesn't handle the case for what if one wasn't present. Maybe instead we should pass in the return Future so repeat won't have to create one? repeat { ... } foreach => \@array, future => Some::Future::Subclass->new; None of these feel overly clear as "the right answer". Do you have any further opinions? -- Paul Evans

Mon Mar 25 16:58:54 2013 The RT System itself - Status changed from 'new' to 'open'

Tue Mar 26 08:45:51 2013 TOSHIOITO [...] cpan.org - Correspondence added

I don't have a clear answer either, but I suppose the following two are good (if not best) options. OPTION 1: Combination of your option (1) and "else" option repeat { ... } foreach => \@array, else => sub { Future->new->fail("None") }; "else" is optional. If @array is empty and "else" is not provided, it throws an exception. That makes sense to me, because in that case there is nothing it can do. PROBLEM of OPTION 1: It is still a bit awkward because an empty @array is treated as a special case. OPTION 2: Make it a class method, like wait_all() method Future->repeat(foreach => \@array, do => sub { ... }); Future->repeat(while => sub { condition() }, do => sub { ... }); That way, we can create the eventual future from the invocant class. In this style, 'while' and 'until' should evaluate their condition block before the 'do' block. That behavior is analogous to Perl's while, until and foreach statements. PROBLEM of OPTION 2: It may be a big change in this module. More work will be necessary than OPTION 1. And it looks cooler to me when Future and Future::Utils are separated. Future already has a lot of methods.

Thu May 09 09:50:05 2013 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Mon Mar 25 01:12:20 2013, TOSHIOITO wrote: Show quoted text

> When Future::Utils::repeat() function is called with foreach => [] > option, the CODE block is executed once with undef value given as the > $item. > > Because this behavior may be counter-intuitive for users, it should at > least be documented.

I have now given this a lot of thought, and have implemented a new paramter called "otherwise", which provides the behaviour to take at the end of the list (including when handling an empty list): $future = repeat { CODE } foreach => ARRAY, otherwise => CODE Calls the "CODE" block once for each value obtained from the array, passing in the value as the first argument (before the previous trial future). When there are no more items left in the array, the "otherwise" code is invoked once and passed the last trial future, if there was one, otherwise "undef" if the list was originally empty. The result of the eventual future will be the result of the future returned from "otherwise". The referenced array may be modified by this operation. $trial_f = $code->( $item, $previous_trial_f ) $final_f = $otherwise->( $last_trial_f ) The "otherwise" code is optional; if not supplied then the result of the eventual future will simply be that of the last trial. It also emphasises that you probably want to supply an 'otherwise' block there; just the choice is left open for backward compatibility. This will be in the next release, with hopefully also a more generic operation that allows you to use a CODE ref as a generic generator of items, instead of iterating a fixed array. -- Paul Evans

Sat May 11 09:29:20 2013 TOSHIOITO [...] cpan.org - Correspondence added

I've checked out Future 0.13. "otherwise" parameter is cool. It's consistent in both cases where the target array is empty or not. However, because "otherwise" is optional, the POD would be more helpful if you clarified what would happen if "otherwise" is omitted and the target array is empty.

Thu Jul 17 06:18:21 2014 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Added further clarification, and fixed the two "empty" cases of foreach => [] and generate => sub {} without an otherwise. -- Paul Evans

Subject:

rt84189.patch

=== modified file 'lib/Future/Utils.pm' --- lib/Future/Utils.pm 2014-06-08 21:57:09 +0000 +++ lib/Future/Utils.pm 2014-07-17 10:11:39 +0000 @@ -206,8 +206,8 @@ Calls the C<CODE> block once for each value obtained from the array, passing in the value as the first argument (before the previous trial future). When there are no more items left in the array, the C<otherwise> code is invoked -once and passed the last trial future, if there was one, otherwise C<undef> if -the list was originally empty. The result of the eventual future will be the +once and passed the last trial future, if there was one, or C<undef> if the +list was originally empty. The result of the eventual future will be the result of the future returned from C<otherwise>. The referenced array may be modified by this operation. @@ -216,7 +216,9 @@ $final_f = $otherwise->( $last_trial_f ) The C<otherwise> code is optional; if not supplied then the result of the -eventual future will simply be that of the last trial. +eventual future will simply be that of the last trial. If there was no trial, +because the C<foreach> list was already empty, then an immediate successful +future with an empty result is returned. =head2 $future = repeat { CODE } foreach => ARRAY, while => CODE, ... @@ -339,7 +341,7 @@ goto &$otherwise; } else { - return $last_trial_f; + return $last_trial_f || Future->done; } }; === modified file 't/34utils-repeat-foreach.t' --- t/34utils-repeat-foreach.t 2014-03-26 15:09:41 +0000 +++ t/34utils-repeat-foreach.t 2014-07-17 10:11:39 +0000 @@ -56,6 +56,20 @@ is( scalar $future->failure, "Nothing to do\n", '$future returns otherwise failure for empty list' ); } +# foreach on empty list +{ + my $future = repeat { die "Not invoked" } foreach => []; + + ok( $future->is_ready, 'repeat {} on empty foreach without otherwise already ready' ); + is_deeply( [ $future->get ], [], 'Result of empty future' ); + + $future = repeat { die "Not invoked" } foreach => [], + otherwise => sub { Future->done( 1, 2, 3 ) }; + + ok( $future->is_ready, 'repeat {} on empty foreach with otherwise already ready' ); + is_deeply( [ $future->get ], [ 1, 2, 3 ], 'Result of otherwise future' ); +} + # foreach while { my $future = try_repeat {

Thu Jul 17 06:18:23 2014 leonerd-cpan [...] leonerd.org.uk - Status changed from 'open' to 'patched'

Thu Jul 17 20:35:47 2014 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Released in 0.29 -- Paul Evans

Thu Jul 17 20:35:49 2014 leonerd-cpan [...] leonerd.org.uk - Status changed from 'patched' to 'resolved'

Thu Jul 17 20:35:50 2014 leonerd-cpan [...] leonerd.org.uk - Fixed in 0.29 added