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.

Sign up for GitHub

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

Jump to bottom

Update react/stream to support 0.7.2 and future 1.x #38

Merged
merged 1 commit into from
Jun 18, 2017
Merged

Update react/stream to support 0.7.2 and future 1.x #38

merged 1 commit into from
Jun 18, 2017

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Jun 12, 2017 •
edited
Loading

Supersedes / closes #35
Implements / closes #37
Implements / closes #30

@WyriHaximus WyriHaximus requested review from jsor and clue June 12, 2017 06:05
WyriHaximus
WyriHaximus commented Jun 12, 2017
View reviewed changes
src/Process.php Outdated
$this->stdin = new WritableResourceStream($this->pipes[0], $loop);
$this->stdin->on('close', function () {
if (is_resource($this->pipes[0])) {
fclose($this->pipes[0]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this is needed until reactphp/stream#107 is merged and tagged

clue
clue reviewed Jun 12, 2017
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README probably also needs an update, otherwise LGTM 👍

src/Process.php Outdated
if (is_resource($that->pipes[0])) {
fclose($that->pipes[0]);
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is work around for reactphp/stream#107? Does it make sense to back port this to older stream release and only target fixed versions here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is, made a comment on it after making the PR #38 (comment) and that makes sense 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed the code removing this and going 0.7.2 and up with react/stream.

@clue clue changed the title Update react/stream to support 0.6, 0.7, and 1.x Update react/stream to support 0.7.2 and future 1.x Jun 16, 2017
clue
clue reviewed Jun 16, 2017
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM, but the README could use an update 👍

For the reference: The unrelated HHVM build error is being addressed via #39.

@WyriHaximus
Copy link
Member Author

Change LGTM, but the README could use an update 👍

Yeah I'm working on that 👍

@WyriHaximus WyriHaximus added easy pick and removed bug labels Jun 16, 2017
@WyriHaximus
Copy link
Member Author

Ping @reactphp/core updated the readme 😎

jsor
jsor reviewed Jun 17, 2017
View reviewed changes
README.md Outdated
@@ -64,16 +64,16 @@ access fields otherwise available through `proc_get_status()`.
### Stream Properties

Once a process is started, its I/O streams will be constructed as instances of
`React\Stream\Stream`. Before `start()` is called, these properties are `null`.
`React\Stream\ReadableResourceStream` as `React\Stream\WritableResourceStream`. Before `start()` is called, these properties are `null`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as -> and

Also, should we use ReadableStreamInterface and WritableStreamInterface instead of the concrete implementations?

README.md Outdated
Once a process terminates, the streams will become closed but not unset.

* `$stdin`
* `$stdout`
* `$stderr`

Each of these implement the underlying
[`DuplexStreamInterface`](https://github.com/reactphp/stream#duplexstreaminterface)
and you can use any of its events and methods as usual:
[`ReadableResourceStream`](https://github.com/reactphp/stream#readablestreaminterface) or [`WritableResourceStream`](https://github.com/reactphp/stream#writablestreaminterface)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment about ReadableStreamInterface and WritableStreamInterface. If we mention the concrete implementations, we should probably also link to their docs (eg. https://github.com/reactphp/stream#readableresourcestream) instead of the interface docs.

README.md Outdated
// …
```

For more details, see the
[`DuplexStreamInterface`](https://github.com/reactphp/stream#duplexstreaminterface).
[`ReadableResourceStream`](https://github.com/reactphp/stream#readablestreaminterface) and [`WritableResourceStream`](https://github.com/reactphp/stream#writablestreaminterface).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comments about ReadableStreamInterface and WritableStreamInterface.

WyriHaximus added a commit to WyriHaximus-labs/child-process that referenced this pull request Jun 17, 2017
@WyriHaximus
As => and reactphp#38 (comment)
db1f46a
WyriHaximus added a commit to WyriHaximus-labs/child-process that referenced this pull request Jun 17, 2017
@WyriHaximus
Use interfaces instead of concrete classes in the documentation react…
ae3dbf0 
…php#38 (comment)
clue
clue reviewed Jun 17, 2017
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README could use some line wraps and the commits should be squashed to a reasonable number, otherwise LGTM 👍

@WyriHaximus WyriHaximus added this to the v0.5.0 milestone Jun 17, 2017
@WyriHaximus
Copy link
Member Author

@clue wrapped the lines. Squashing in a moment 😎

@WyriHaximus
Update react/stream to support 0.6, 0.7, and 1.x
4ef64e9 
PHP 5.3 fix

Updated to react/stream:^0.7.2

Removed unused React\Stream\Stream import

Updated readme to the updated streams

As => and reactphp#38 (comment)

Use interfaces instead of concrete classes in the documentation reactphp#38 (comment)

Line folding
@WyriHaximus WyriHaximus force-pushed the upgrade-react-stream branch from ff3828a to 4ef64e9 Compare June 17, 2017 16:39
@WyriHaximus
Copy link
Member Author

Squash commits 😎

clue
clue approved these changes Jun 17, 2017
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks! :shipit:

@WyriHaximus WyriHaximus merged commit 5d0047f into reactphp:master Jun 18, 2017
@clue clue mentioned this pull request Aug 18, 2017
Merged
@clue clue mentioned this pull request Jan 13, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsor jsor jsor approved these changes

@clue clue clue approved these changes

Assignees
No one assigned
Labels
easy pick help wanted maintenance
Projects
None yet
Milestone
v0.5.0
Development

Successfully merging this pull request may close these issues.

Update stream dependency to 0.7 STDIO streams should be readable or writable, but not duplex
3 participants
@WyriHaximus @jsor @clue