PeclEvLoop: Add new PeclEvLoop (PECL ext-ev) #97
Conversation
The existing libev driver seems to function only with a non-official, out of date libev extension for PHP. This PeclEvLoop class is a very similar clone of the LibEvLoop library but for the officially documented libev extension. Changes are mostly to constants and function parameter order.
|
Sorry, I forgot to add travis configuration for PECL ev extension (which is necessary for |
|
@kaduev13 tbh no need to close your PR for that, just add another commit fixing that. Or just squash it in your last commit. (@clue is our resident Git guru) |
|
@WyriHaximus ok, thank you. I'm very new to open-source, but I will learn fast ;) Btw, it's fixed now. |
src/PeclEvLoop.php
Outdated
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function tick() |
There was a problem hiding this comment.
Note, that tick() has been removed from the interface (see #72).
src/PeclEvLoop.php
Outdated
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function addReadStream($stream, callable $listener) |
There was a problem hiding this comment.
The docblocks containing just {@inheritdoc} can be removed.
There was a problem hiding this comment.
@jsor, no problem, I can remove it, but {@inheritdoc} makes inheritance explicit (link to PSR-5 inheritance section). Is it a reactphp project convention?
There was a problem hiding this comment.
Since this method implements the interface, it's explicit that the docblock is inherited. We tend to avoid docblocks since they are often redundant and become easily out of date. With type hints and return types in PHP 7, they will become even more obsolete (at least 99.9% of the time) :)
There was a problem hiding this comment.
@jsor, ok, thank you for the explanation 😃 , I agree with your arguments (but I just looked through LibEvLoop, StreamSelectLoop and other loops sources and there are a lot of "only @inheritdoc" PHPDocs).
I removed unnecessary @inheritdoc blocks from PeclEvLoop in 7128aaf.
Btw, I think that it'll be better to cleanup the history of this PR before merging it into master (cause of a lot of little fix commits). I will do it tomorrow morning (UTC).
tests/PeclEvLoopTest.php
Outdated
| /** | ||
| * @author Ivan Kalita kaduev13@gmail.com | ||
| */ | ||
|
|
There was a problem hiding this comment.
We usually don't add @author doc blocks.
|
ping @WyriHaximus and @clue |
|
ping @clue |
| use SplObjectStorage; | ||
|
|
||
| /** | ||
| * @see https://bitbucket.org/osmanov/pecl-ev/overview |
| * | ||
| * @return \Closure | ||
| */ | ||
| private function getStreamListenerClosure($stream, callable $listener) { |
There was a problem hiding this comment.
Minor nitpick: Line break before { (PSR-2)
|
@clue Thank you for the corrections, I'll fix these issues on this weekend 😃 |
|
@kaduev13 any luck with those corrections? |
clue
changed the title
|
Superseded by #148 👍 |
Improved and (just a little bit) refactored #25.