-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Proposal: Add iterable\any(iterable $input, ?callable $cb=null), all(...), none(...), find(...), reduce(...) #6053
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
Closed
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
5aadb06
Add PHP\iterable\any(...) and all(iterable $input, ?callable $cb=null)
bugreportuser b300cbe
Use Z_PARAM_ITERABLE macro, change namespace
TysonAndre 7f3cf58
Add iterable\none(iterable $iterable, ?callable $callback=null):bool
TysonAndre 06310e1
Implement iterator\reduce($carry, $item): mixed
TysonAndre 6a01897
Implement iterable\find($iterable, $callback, $default=null): mixed
TysonAndre 209e429
Remove unnecssary inlining
morrisonlevi 8217ce5
Remove $initial from reduce; throw on empty
morrisonlevi 4d02c6f
Fix enum type
morrisonlevi c826269
Fix test failure
TysonAndre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
--TEST-- | ||
Test all() function | ||
--FILE-- | ||
<?php | ||
|
||
use function iterable\all; | ||
|
||
/* | ||
Prototype: bool all(array $array, mixed $callback); | ||
Description: Iterate array and stop based on return value of callback | ||
*/ | ||
|
||
function is_int_ex($item) | ||
{ | ||
return is_int($item); | ||
} | ||
|
||
echo "\n*** Testing not enough or wrong arguments ***\n"; | ||
|
||
function dump_all(...$args) { | ||
try { | ||
var_dump(all(...$args)); | ||
} catch (Error $e) { | ||
printf("Caught %s: %s\n", $e::class, $e->getMessage()); | ||
} | ||
} | ||
|
||
dump_all(); | ||
dump_all(true); | ||
dump_all([]); | ||
dump_all(true, function () {}); | ||
dump_all([], true); | ||
|
||
echo "\n*** Testing basic functionality ***\n"; | ||
|
||
dump_all([1, 2, 3], 'is_int_ex'); | ||
dump_all(['hello', 1, 2, 3], 'is_int_ex'); | ||
$iterations = 0; | ||
dump_all(['hello', 1, 2, 3], function($item) use (&$iterations) { | ||
++$iterations; | ||
return is_int($item); | ||
}); | ||
var_dump($iterations); | ||
|
||
echo "\n*** Testing edge cases ***\n"; | ||
|
||
dump_all([], 'is_int_ex'); | ||
|
||
echo "\nDone"; | ||
?> | ||
--EXPECT-- | ||
*** Testing not enough or wrong arguments *** | ||
Caught ArgumentCountError: iterable\all() expects at least 1 argument, 0 given | ||
Caught TypeError: iterable\all(): Argument #1 ($iterable) must be of type iterable, bool given | ||
bool(true) | ||
Caught TypeError: iterable\all(): Argument #1 ($iterable) must be of type iterable, bool given | ||
Caught TypeError: iterable\all(): Argument #2 ($callback) must be a valid callback or null, no array or string given | ||
|
||
*** Testing basic functionality *** | ||
bool(true) | ||
bool(false) | ||
bool(false) | ||
int(1) | ||
|
||
*** Testing edge cases *** | ||
bool(true) | ||
|
||
Done |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
--TEST-- | ||
Test all() function | ||
--FILE-- | ||
<?php | ||
|
||
use function iterable\all; | ||
|
||
/* | ||
Prototype: bool all(array $array, ?callable $callback = null, int $use_type = 0); | ||
Description: Iterate array and stop based on return value of callback | ||
*/ | ||
|
||
function is_int_ex($item) | ||
{ | ||
return is_int($item); | ||
} | ||
|
||
function dump_all(...$args) { | ||
try { | ||
var_dump(all(...$args)); | ||
} catch (Error $e) { | ||
printf("Caught %s: %s\n", $e::class, $e->getMessage()); | ||
} | ||
} | ||
|
||
|
||
echo "\n*** Testing not enough or wrong arguments ***\n"; | ||
|
||
dump_all(new ArrayIterator()); | ||
dump_all(new ArrayIterator(), true); | ||
|
||
echo "\n*** Testing basic functionality ***\n"; | ||
|
||
dump_all(new ArrayIterator([1, 2, 3]), 'is_int_ex'); | ||
dump_all(new ArrayIterator(['hello', 1, 2, 3]), 'is_int_ex'); | ||
$iterations = 0; | ||
dump_all(new ArrayIterator(['hello', 1, 2, 3]), function($item) use (&$iterations) { | ||
++$iterations; | ||
return is_int($item); | ||
}); | ||
var_dump($iterations); | ||
|
||
echo "\n*** Testing edge cases ***\n"; | ||
|
||
dump_all(new ArrayIterator(), 'is_int_ex'); | ||
|
||
echo "\nDone"; | ||
?> | ||
--EXPECT-- | ||
*** Testing not enough or wrong arguments *** | ||
bool(true) | ||
Caught TypeError: iterable\all(): Argument #2 ($callback) must be a valid callback or null, no array or string given | ||
|
||
*** Testing basic functionality *** | ||
bool(true) | ||
bool(false) | ||
bool(false) | ||
int(1) | ||
|
||
*** Testing edge cases *** | ||
bool(true) | ||
|
||
Done |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
--TEST-- | ||
Test any() function | ||
--FILE-- | ||
<?php | ||
|
||
use function iterable\any; | ||
|
||
/* | ||
Prototype: bool any(array $iterable, mixed $callback); | ||
Description: Iterate array and stop based on return value of callback | ||
*/ | ||
|
||
function is_int_ex($nr) | ||
{ | ||
return is_int($nr); | ||
} | ||
|
||
echo "\n*** Testing not enough or wrong arguments ***\n"; | ||
|
||
function dump_any(...$args) { | ||
try { | ||
var_dump(any(...$args)); | ||
} catch (Error $e) { | ||
printf("Caught %s: %s\n", $e::class, $e->getMessage()); | ||
} | ||
} | ||
|
||
dump_any(); | ||
dump_any(true); | ||
dump_any([]); | ||
dump_any(true, function () {}); | ||
dump_any([], true); | ||
|
||
echo "\n*** Testing basic functionality ***\n"; | ||
|
||
dump_any(['hello', 'world'], 'is_int_ex'); | ||
dump_any(['hello', 1, 2, 3], 'is_int_ex'); | ||
$iterations = 0; | ||
dump_any(['hello', 1, 2, 3], function($item) use (&$iterations) { | ||
++$iterations; | ||
return is_int($item); | ||
}); | ||
var_dump($iterations); | ||
|
||
echo "\n*** Testing second argument to predicate ***\n"; | ||
|
||
dump_any([1, 2, 3], function($item, $key) { | ||
var_dump($key); | ||
return false; | ||
}); | ||
|
||
echo "\n*** Testing edge cases ***\n"; | ||
|
||
dump_any([], 'is_int_ex'); | ||
|
||
dump_any(['key' => 'x'], null); | ||
|
||
echo "\nDone"; | ||
?> | ||
--EXPECT-- | ||
*** Testing not enough or wrong arguments *** | ||
Caught ArgumentCountError: iterable\any() expects at least 1 argument, 0 given | ||
Caught TypeError: iterable\any(): Argument #1 ($iterable) must be of type iterable, bool given | ||
bool(false) | ||
Caught TypeError: iterable\any(): Argument #1 ($iterable) must be of type iterable, bool given | ||
Caught TypeError: iterable\any(): Argument #2 ($callback) must be a valid callback or null, no array or string given | ||
|
||
*** Testing basic functionality *** | ||
bool(false) | ||
bool(true) | ||
bool(true) | ||
int(2) | ||
|
||
*** Testing second argument to predicate *** | ||
Caught ArgumentCountError: Too few arguments to function {closure}(), 1 passed and exactly 2 expected | ||
|
||
*** Testing edge cases *** | ||
bool(false) | ||
bool(true) | ||
|
||
Done |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this one? I'd rather add filter, map, and flatmap if we need more functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find() is also useful and many other programming languages include both (js, haskell, etc.)
For example, if you have an array of a million elements and only want the first match, it is much more efficient to call find() if the iterable contains a matching value (and there would be less service calls and db calls) compared to
reset(array_filter(...))
Additionally, filter() and map() would be waiting on the existence of CachedIterable when it starts, because Traversables can have repeated keys
yield 'key' => 1; yield 'key' => 2;
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not require a CachedIterable; they can return any iterator.
Edit: on second thought, they should not return a cached iterable. These routines are often chained together; if every piece of the chain caches their results, it will balloon memory usage.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that there's nothing in the standard library yet that supports rewinding, counting, and especially arbitrary offset access
It seems much more difficult to use without the support for rewindability, random/repeated offset access, countability, etc.
But yes, I suppose you could hide the implementation entirely with InternalIterator and only support a single iteration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CachedIterable is basically the name chosen for a rewindable immutable key-value sequence. It isn't cached permanently, it has a regular object lifetime. I'm referring to https://wiki.php.net/rfc/cachediterable
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I am specifically saying they should not return this object. It will ballon memory usage to do it that way. Think about it; you have a map + filter plus some terminator like
first_n
with n=100. Filter and map will each hold at least 100 values in memory that shouldn't be there while calculating thefirst_n
.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterables are made from arrays and generators. Since there is already a large body of functions which work with arrays, it seems reasonable to assume they would use an
Iterable
part of the standard library because they also use iterators/generators. One of the main points of generators is to reduce memory. We shouldn't add a corpus of iterable functions that will increase memory; that works directly against the goals of the iterable feature!No, it's much better to compose something if you want it to be eager, e.g.
$foo = CachedIterable::new(map(...$args));
We can include this in the examples in the docs for filter, map, etc.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterables are made from arrays and Traversable objects such as SplObjectStorage, ArrayObject, user-defined data structures, and Generators
In common use cases, the memory increase may be small, especially if the lifetime of the variable or temporary expression holding the result is short.
Additionally, a lazy iterable would require keeping a reference to the previous (possibly lazy) iterable, and the iterables/arrays those reference - if the initial iterable is larger than the result then eagerly evaluating
$var = map($cb, temporary())
would end up saving memoryThat seems error prone and I'm personally opposed to that.
PHP has typically been imperitive rather than functional, and focused on "cater[ing] to the skill-levels and platforms of a wide range of users" as RFC authors are repeatedly reminded in https://wiki.php.net/rfc/template and my interpretation of that is that imperitive would be much more acceptable (aside: The loosely typed language part seems less applicable nowadays)
Lazy data structures would be easy to misuse (consume twice, attempt to serialize or encode, (or var_dump or inspect with Xdebug), easier to attempt to log the full iterable(consume twice) etc) without (or even with) linters and static analyzers, so this really doesn't seem like catering to a wide range of users.
Explicitly using a different family of functions to act on generators internally would probably make more sense than being the default, e.g. https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html#findFirst-- and https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html (Streams are separate from java.util.Collection in java, javascript eagerly evaluates https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Map, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the eager version be longer than the lazy version (instead of shorter or the same length) would also encourage the use of the lazy version, which I'd objected to for being error prone and easy to misuse.