Added missing functions #6
Conversation
kyl0b1te
commented
Oct 6, 2017
kyl0b1te
commented
- changed the way of yams parsing (added new dependency)
- added missing beanstalk functions
src/BeanstalkClient.php
Outdated
|
|
||
| switch ($type) { | ||
| case "PAUSED": | ||
| return true; |
There was a problem hiding this comment.
I guess it can just return null if the return value doesn't have any other meaning, right?
There was a problem hiding this comment.
For me, true here mean that operation was executed and finished successfully. Null is not really informative here, IMO
There was a problem hiding this comment.
true isn't either, as the successful resolution of the promise already indicates success. It's just like a void function in blocking PHP.
There was a problem hiding this comment.
Hmm... maybe you're right. But I'm still not sure :)
There was a problem hiding this comment.
So you will not merge without this true to null conversion ?
There was a problem hiding this comment.
I can also change it afterwards. ;-) Could you provide a code sample that examines the return value and acts based on it?
There was a problem hiding this comment.
Any function call with remote communication that should change server state. Is not really helpful use extensions for such cases every time.
Image that:
function setServerAvailablity(int $code) { ... }
will return null. Is it informative ?
Compare it with true
src/BeanstalkClient.php
Outdated
| foreach ($source as $stat) { | ||
| if ($stat == '---' || empty($stat)) { | ||
| continue; | ||
| public function getTubesList(): Promise { |
There was a problem hiding this comment.
I'm not completely sure about it. For me, its more predictable to have a getTubesList() and getWatchedTubesList() methods. What do you think ?
There was a problem hiding this comment.
I think listWatchedTubes() reads slightly better, but I don't have a strong preference for either.
@markkimsal Thoughts?
|
Could you please rebase your changes onto the current master instead of merging it? It might be easier to just create a new branch and cherry-pick the few commits. 👍 |
|
Is there any updates ? :) |
src/BeanstalkClient.php
Outdated
| }); | ||
| } | ||
|
|
||
| public function getUsingTube(): Promise { |
There was a problem hiding this comment.
Could you rename that one to getUsedTube()?
There was a problem hiding this comment.
Really? What name you want to see here ?
There was a problem hiding this comment.
getUsedTube(), as mentioned. 😄
Other than that it looks perfectly fine now. 👍
|
Seems like you rebased one commit too early (c952a54), do you want to fix that or should I just go ahead and merge it? |
|
@kelunik I think better you to do it |
|
I'll just create a merge commit, too lazy to fix it up now. The history isn't too unclean. Thanks for your contribution! |
