search

amphp / process

An async process dispatcher for Amp.
MIT License
229 stars 32 forks source link

Added some test coverage and fixed the structure #3

Closed skedone closed 9 years ago

skedone commented 9 years ago
bwoebi commented 9 years ago

Mostly looks good for me, I'm just not a fan of one-file directories, hence no lib/ here.

Also, as said, do we really need status() ? It's still superfluous. You get everything you need from pid() and the return value of the promise. It has no additional value, except making the API more complicated.

Just wondering, while you added getCommand(), why don't you expose what the options are? But generally, I think, it's better to actually just inject the command itself somewhere if that code needs to be aware of the command instead of wrapping it in a Process object. If you want to encapsulate the command, wrap it and inject the Process object; if you want to get the command, just inject the command and have the called code wrap it in the Process object.

kelunik commented 9 years ago

Mostly looks good for me, I'm just not a fan of one-file directories, hence no lib/ here.

Consistency > personal preference here IMO.

Also, as said, do we really need status() ?

Maybe we should include a getHandle instead.

Just wondering, while you added getCommand(), why don't you expose what the options are?

It's a good idea to have getCommand and getOptions.

But generally, I think, it's better to actually just inject the command itself somewhere if that code needs to be aware of the command instead of wrapping it in a Process object. If you want to encapsulate the command, wrap it and inject the Process object; if you want to get the command, just inject the command and have the called code wrap it in the Process object.

I don't get what you're trying to say here.

pid should be getPid IMO.

Additionally, there are whitespace issues, could you convert everything to spaces instead of just half of it?

bwoebi commented 9 years ago

Let's not do all the whitespace issues in that PR, that can be a separate commit later then.

I'm saying that if another context needs to be aware of the command, why don't you just inject the string command instead of wrapping it first in a Process instance? So, making the getCommand() method actually superfluous.

We indeed might expose the resource, just like we also have SpecificReactor::getLoop(). But honestly, is there any special use case where you'd directly need the resource?

kelunik commented 9 years ago

Calling e.g. proc_get_status when it's not part of the API.

Why should we wrap it again instead of exposing a simple getter for a property that's already there? You might need it when having a lot of processes in an array or something.

skedone commented 9 years ago

I have an SplObejct full of processes reference and I use that getter in some place in my project to discriminate/check those processes. I agree with a simpler setter, I fork processes and then I use sockets to get informations about those background processes: for me it is useful (unless the property remains private, I need a getter and I can not simply extends the class).

I agree with all the points Niklas raised, the naming part should be different and conventions over preferences. On 11 Sep 2015 21:04, "Bob Weinand" notifications@github.com wrote:

Let's not do all the whitespace issues in that PR, that can be a separate commit later then.

I'm saying that if another context needs to be aware of the command, why don't you just inject the string command instead of wrapping it first in a Process instance? So, making the getCommand() method actually superfluous.

We indeed might expose the resource, just like we also have SpecificReactor::getLoop(). But honestly, is there any special use case where you'd directly need the resource?

— Reply to this email directly or view it on GitHub https://github.com/amphp/process/pull/3#issuecomment-139632523.

kelunik commented 9 years ago

I still think that Process should be in /lib.