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

Change method signature to return iterable? #49

Closed
romm opened this issue Apr 7, 2020 · 3 comments
Closed

Change method signature to return iterable? #49

romm opened this issue Apr 7, 2020 · 3 comments

Comments

@romm
Copy link

romm commented Apr 7, 2020

This is only a quick thought, maybe there would be some side-effects that I can not see right now.

Changing the return type of the two methods to iterable would ease the use of generators, for instance something like this:

final class AggregateServiceProvider implements ServiceProviderInterface
{
    /** @var ServiceProviderInterface[] */
    private array $serviceProviders;

    public function __construct(ServiceProviderInterface ...$serviceProviders)
    {
        $this->serviceProviders = $serviceProviders;
    }
    
    public function getFactories(): iterable
    {
        foreach ($this->serviceProviders as $serviceProvider) {
            yield from $serviceProvider->getFactories();
        }
    }
    
    public function getExtensions(): iterable
    {
        foreach ($this->serviceProviders as $serviceProvider) {
            yield from $serviceProvider->getExtensions();
        }
    }
}

What do you think?

@bnf
Copy link

bnf commented Apr 8, 2020

I like the idea and thought about proposing that as well, there is one thing I wasn't sure about:

maybe there would be some side-effects that I can not see right now

There is one drawback of iterable in constrast to array for consumers (containers):
An iterable can only be iterated once, as a generator may be closed after the first iteration.
That also means: No array-style access nor copy operations with + .

Still, I think iterable would be good. The spec should make it cristal clear that consuming containers should be unit tested against generators to ensure they do only perform iterations on getFactories() and getExtensions(), and only once.

@mindplay-dk
Copy link
Collaborator

There is one drawback of iterable in constrast to array for consumers (containers):
An iterable can only be iterated once, as a generator may be closed after the first iteration.
That also means: No array-style access nor copy operations with + .

This sounds like it might invite problems?

Why would you need generators for this? What's the advantage over arrays?

Aren't containers basically always going to apply the entire set of factories and extensions all at once?

@mindplay-dk
Copy link
Collaborator

Since this idea appears to invite problems, I am going to close this issue. No real reason was given for this suggestion. If you did have generators for some reason, since they are going to be consumed by the container in full anyway, you can simply convert them to arrays. I would imagine this is a very rare issue anyway, as generators are typically used for sequential data rather than sets/maps.

If you disagree, please feel free to reopen and clarify the requirement.

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