Skip to content

Fix invalid use for ConcurrentIterableIterator#101

Merged
trowski merged 2 commits intoamphp:v2from
ivanpepelko:v2
Dec 23, 2022
Merged

Fix invalid use for ConcurrentIterableIterator#101
trowski merged 2 commits intoamphp:v2from
ivanpepelko:v2

Conversation

@ivanpepelko
Copy link
Contributor

Also added the test for php engine iterables. The exception was not caught in tests because instance of ConcurrentIterableIterator was always passed to ReadableIterableStream constructor in tests and never hit the branch for other type of iterables.

Also added the test for php engine iterables. The exception was not
caught in tests because instance of ConcurrentIterableIterator was always passed
to ReadableIterableStream constructor in tests and never hit the branch for
other type of iterables.
@trowski
Copy link
Member

trowski commented Dec 23, 2022

ConcurrentIteratorIterator is marked internal now. Can you please change the constructor to the following instead and remove the use statement.

    /**
     * @param iterable<mixed, string> $iterable
     */
    public function __construct(iterable $iterable)
    {
        $this->iterator = $iterable instanceof ConcurrentIterator
            ? $iterable
            : Pipeline::fromIterable($iterable)->getIterator();

        $this->onClose = new DeferredFuture;
    }

@trowski trowski merged commit 16ed0e9 into amphp:v2 Dec 23, 2022
@ivanpepelko
Copy link
Contributor Author

I noticed your comment just now... Well, carry on and happy holidays!

@trowski
Copy link
Member

trowski commented Dec 23, 2022

Yep, sorry I'm not usually that impatient, but I wanted to work on this and dependent libraries this weekend and this was actually holding things up. ReadableIterableStream is heavily used in dependents.

Thanks and happy holidays to you too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants