-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
Use yield for iterable results #1198
Comments
I really like the idea! My concern is about BC break and migration path. For sure, the signature will change and we need a new major version to fix it. version 1.0 private function getKeys(): array
{
trigger_deprecation('method getKeys will be removed use getKeysIterable');
return iterator_to_array($this->getKeysIterable();
}
private function getKeysIterable(): iterable
{
yield $...;
} version 2.0 private function getKeysIterable(): iterrable
{
trigger_deprecation('method getKeysIterable will be removed use getKeys');
return $this->getKeys();
}
private function getKeys(): iterable
{
yield $...;
} version 3.0 private function getKeys(): iterable
{
yield $...;
} version 1.0 private function getKeys($flag): array|iterable
{
if (!$flag) {
trigger_deprecation('method getKeys will return an iterable set parameter $flag=true to remove this deprecation');
return iterator_to_array($this->getKeys(true);
}
yield $...;
} version 2.0 private function getKeys($flag): iterable
{
if ($flag) {
trigger_deprecation('parameter $flag is not needed anymore, remove it');
}
yield $...;
} version 3.0 private function getKeys(): iterable
{
yield $...;
} @nicolas-grekas any idea? |
Given that the decoding of data will still be synchronous (as we parse the response body at once), I don't see the benefit here:
So to me, the (dubious) benefits don't outweight the cost. Note that paginators (where getting the full list requires fetching multiple pages) already use generators. So this issue is not about them. |
To keep the library truly "async", I think we should use
yield
instead of constructing the arrays when listing response items.For example,
\AsyncAws\Iam\Result\ListUsersResponse::populateResultUserListType()
(and many other) could benefit from this:This would result in better performance for people who are iterating over the items and returning prematurely. That's because whole
new User(...)
will only happen when it's actually needed.Asking for thoughts here before creating a PR as it would change quite a lot of classes.
Potential BC breaks:
Methods with
: array
return typehint would require their signature to be changed to: iterable
.One of the examples would be
\AsyncAws\AppSync\Result\ListApiKeysResponse::getApiKeys()
.Users who rely on
array
there would need to use[...$response->getApiKeys()]
oriterator_to_array()
if they still want to get them as array.The responses returning
Traversable
oriterable
could benefit from this change without any BC breaks though.Keeping in mind these BC break risks, this change could target v2 just to be safe.
The text was updated successfully, but these errors were encountered: