-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Update docblock for Process::$pipes #80
Conversation
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.
@gdejong Thanks for spotting, the existing definition is indeed incorrect! 👍
It looks like this should be array<ReadableStreamInterface|WritableStreamInterface>
instead of ReadableStreamInterface[]|WritableStreamInterface[]
.
This subtle different means it can contain a mix of both readable and writable streams, instead of an array of either just readable or just writable streams.
Good catch! I'll update the PR! |
Found this when running Psalm in a project of mine that is using reactphp/child-process. ``` Possibly undesired iteration over regular object React\Stream\ReadableStreamInterface ``` for the code: ``` foreach ($this->process->pipes as $pipe) { ``` The docblock now better reflects the fact that `$pipes` is an array of either `ReadableStreamInterface` or `WritableStreamInterface`.
Updated (amended to the same commit). Unfortunately the current version of PHPStorm ( For now it means that code like |
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.
@gdejong Thanks for the update, the changes LGTM!
If static analysis and IDE autocompletion is an issue, you can always add an additional assert($pipe instanceof ReadableStreamInterface || $pipe instanceof WritableStreamInterface)
in front of the respective method call.
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.
Nice catch, thank you 👍
Found this when running Psalm in a project of mine that is using
reactphp/child-process
.for the code:
The docblock now better reflects the fact that
$pipes
is an array of eitherReadableStreamInterface
orWritableStreamInterface
.