The Wayback Machine - https://web.archive.org/web/20200920234931/https://github.com/reactphp/event-loop/pull/110
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

Documentation for advanced stream API #110

Merged
merged 5 commits into from Oct 14, 2017
Merged

Conversation

@clue
Copy link
Member

clue commented Oct 13, 2017

The stream API is currently mostly undocumented. This PR adds documentation for the existing stream API and then subsequently removes the unneeded and undocumented loop argument that was previously passed to stream listeners.

This is a subtle BC break. Empirical evidence (including our examples, tests and other components) suggest that most consumers will not be affected by this. The added documentation ensures that we now follow a strict API and will not introduce a BC break in the future.

Trying to create documentation for this API is not exactly trivial, as it's very low-level and has existed in this form almost since its inception. Given that most consumers SHOULD NOT use this API at all, I've marked the stream API as advanced and linked to the Stream component instead.

Performance improvement is not a major motivation here, but shows a negligible improvement anyway (running examples 92 and 94).

If you want to review, consider also looking at the individual commits.

Builds on top of #100 and #102.
Resolves / closes #87

@clue clue added this to the v0.5.0 milestone Oct 13, 2017
@WyriHaximus WyriHaximus requested review from jsor and WyriHaximus Oct 13, 2017
README.md Outdated
register a listener to be notified when a stream is ready to write.

The first parameter MUST be a valid stream resource that supports
checking whether it is ready to read by this loop implementation.

This comment has been minimized.

@jsor

jsor Oct 13, 2017 Member

...it is ready to write by this...

This comment has been minimized.

@clue

clue Oct 13, 2017 Author Member

Thanks for spotting, fixed and amended! :shipit:

@jsor
jsor approved these changes Oct 13, 2017
Copy link
Member

WyriHaximus left a comment

:shipit:

@WyriHaximus WyriHaximus merged commit 6090eb7 into reactphp:master Oct 14, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@clue clue deleted the clue-labs:streams branch Oct 14, 2017
@kelunik

This comment has been minimized.

Copy link
Contributor

kelunik commented on tests/StreamSelectLoopTest.php in 8997565 Oct 15, 2017

That breaks PHP 5.3 compat, right?

This comment has been minimized.

Copy link
Member Author

clue replied Oct 15, 2017

This PR is targeted for the v0.5.0 release, PHP 5.3 support has already been dropped with the v0.4.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.