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

Change how the program listening are killed #17

Merged
merged 1 commit into from
Apr 27, 2016
Merged

Change how the program listening are killed #17

merged 1 commit into from
Apr 27, 2016

Conversation

aledeg
Copy link
Contributor

@aledeg aledeg commented Apr 26, 2016

Following recommendations from http://unix.stackexchange.com/a/8927, I introduced a softer way of terminating the programs listening on the wanted port.

Following recommendations from http://unix.stackexchange.com/a/8927, I introduced a softer way of terminating the programs listening on the wanted port.
@ddelnano
Copy link
Owner

Nice I was not aware of that. From a quick glance this lgtm, but will probably wait off on merging this until later tonight when I am off work and can look at it.

@aledeg
Copy link
Contributor Author

aledeg commented Apr 26, 2016

sure

@aledeg
Copy link
Contributor Author

aledeg commented Apr 26, 2016

Quick question.
I was checking the code and find out that the method to kill any php application running on the reserved port is commented out by default. Is there any reason for that?
It might be helpful to be able to trigger it with some parameter.

@ddelnano
Copy link
Owner

yea so when I was developing this heavily I ran into the problem alot. Then once I published it and used the package myself I never had any issues so that was why I commented it out.

I meant to revisit it at some point but I forgot.

@ddelnano
Copy link
Owner

Maybe a flag should be added like -f or --force and that would trigger the change you made?

@aledeg
Copy link
Contributor Author

aledeg commented Apr 27, 2016

That could be helpful in some cases.

@ddelnano
Copy link
Owner

That would allow all the current users to upgrade and use the package the exact same way with no change to functionality.

So I will merge this as is and I will add that flag / option myself or if you want to update your PR with that change before I merge it thats fine too. Just so you are aware.

@aledeg
Copy link
Contributor Author

aledeg commented Apr 27, 2016

I'd rather let you add the flag.
As you know more the code base than I, it will be faster this way !

@ddelnano
Copy link
Owner

Ok cool, merging.

@ddelnano ddelnano merged commit 5431561 into ddelnano:master Apr 27, 2016
@aledeg aledeg deleted the server-kill branch May 4, 2016 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants