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

Allow start/restart/stop single service #874

Merged
merged 3 commits into from Jan 6, 2020

Conversation

ahmedash95
Copy link
Contributor

Sometimes all we just need is to restart nginx or php-fpm during development. so having a way to restart a specific service saves some time instead of waiting for dns-masq, php and nginx to restart

@drbyte
Copy link
Contributor

drbyte commented Dec 27, 2019

While my initial reaction to this was that it's not needed, I have found myself reaching for it several times in the past couple weeks.

While I'm not a fan of the duplicate code it adds, it's not unbearable (and Valet already does some duplicate-code anyway, and this PR is correctly following the existing patterns in Valet).

So, I'm okay with this one.

👍

@ahmedash95
Copy link
Contributor Author

ahmedash95 commented Dec 29, 2019

While my initial reaction to this was that it's not needed, I have found myself reaching for it several times in the past couple weeks.

While I'm not a fan of the duplicate code it adds, it's not unbearable (and Valet already does some duplicate-code anyway, and this PR is correctly following the existing patterns in Valet).

So, I'm okay with this one.

👍

Thanks for your comment. I hate duplicate code too. but as the file is too small and it's not too complicated with repeated lines I think it's okay to have it like the PR now as it follows the already used pattern in Valet.

@ahmedash95 ahmedash95 changed the title Allow start/restart/stop with a specific service Allow start/restart/stop single service Dec 29, 2019
Copy link
Member

@mattstauffer mattstauffer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR! A few requests

cli/valet.php Outdated Show resolved Hide resolved
cli/valet.php Outdated Show resolved Hide resolved
cli/valet.php Outdated Show resolved Hide resolved
cli/valet.php Outdated Show resolved Hide resolved
cli/valet.php Outdated Show resolved Hide resolved
cli/valet.php Outdated Show resolved Hide resolved
cli/valet.php Outdated Show resolved Hide resolved
cli/valet.php Outdated Show resolved Hide resolved
cli/valet.php Outdated Show resolved Hide resolved
@ahmedash95
Copy link
Contributor Author

Hey @mattstauffer , thanks for the review. I've fixed all style issues.
Thanks

cli/valet.php Outdated Show resolved Hide resolved
Co-Authored-By: Matt Stauffer <mattstauffer@users.noreply.github.com>
@mattstauffer mattstauffer merged commit 4b76778 into laravel:master Jan 6, 2020
@ahmedash95 ahmedash95 deleted the patch-1 branch January 6, 2020 18:51
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

3 participants