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

[10.x] Add pipe function to Process layer #46527

Merged
merged 11 commits into from
Apr 6, 2023

Conversation

WendellAdriel
Copy link
Contributor

@WendellAdriel WendellAdriel commented Mar 20, 2023

This is a follow-up of #46239 which had some issues with the branching.

This PR adds a new function pipe to the Process layer that receives an array of processes and runs them in sequence passing the output of the previous process as the input for the next one.

The syntax will be like this:

$pipe = Process::pipe(function ($pipe) {
    $pipe->command('cat test.txt'),
    $pipe->command('grep -i "foo"'),
});

$pipe->run()->output(); // "foo"

Taking into consideration that the content of test.txt will be:

Hello, world
foo

This can be very handy when we need to chain processes that rely on the result of the previous one to run correctly.

@WendellAdriel
Copy link
Contributor Author

@driesvints @taylorotwell
Here's the new PR with the git issues fixed and also the new Exception wrapping the errors while running the pipe.

@WendellAdriel
Copy link
Contributor Author

@driesvints @taylorotwell this failed a test that's not related to my changes, do I need to do anything else here?

@driesvints
Copy link
Member

@WendellAdriel I retriggered those

@WendellAdriel
Copy link
Contributor Author

@WendellAdriel I retriggered those

Thank you so much!!! 😉

@WendellAdriel
Copy link
Contributor Author

@taylorotwell is there anything else missing that I need to work on for this?

@taylorotwell
Copy link
Member

Your usage example shows you passing an array to pipe, but the pipe methods expects a callable in the code?

@WendellAdriel
Copy link
Contributor Author

@taylorotwell oooh, I forgot to update the example, because at first it was like that, I'm going to update the example.

@WendellAdriel
Copy link
Contributor Author

Your usage example shows you passing an array to pipe, but the pipe methods expects a callable in the code?

I just updated the example in the PR description!

@taylorotwell
Copy link
Member

Do we actually need a ProcessPipeException? Can we just re-use ProcessFailedException?

@taylorotwell
Copy link
Member

taylorotwell commented Apr 5, 2023

Another point for discussion: should we always throw on failure? Or, if there is a failure should we return the last failed process and let the user decide whether they want to throw(), just like the default behavior on normal processes?

@WendellAdriel
Copy link
Contributor Author

That's a good point, I think we can just return the last failed process instead now that you mention it.
I'm going to update the PR.

@WendellAdriel
Copy link
Contributor Author

@taylorotwell I updated the code to return the last failed process, removed the ProcessPipeException class and added a new test case to check for a failed process pipe run.

@taylorotwell
Copy link
Member

Another thing - we allow you to add things to the pipe with a key (like pools), but we never pass that key to the output handler like we do with pooled proesses, this means if you pass as output callback to pipe's run method, you can't tell which process the output is from.

@WendellAdriel
Copy link
Contributor Author

Another thing - we allow you to add things to the pipe with a key (like pools), but we never pass that key to the output handler like we do with pooled proesses, this means if you pass as output callback to pipe's run method, you can't tell which process the output is from.

@taylorotwell thanks for the heads up, I missed that in the implementation 😅. I just updated the PR with a new commit for that.

@taylorotwell taylorotwell merged commit 5826904 into laravel:10.x Apr 6, 2023
@taylorotwell
Copy link
Member

Thanks!

@WendellAdriel WendellAdriel deleted the process-pipe-2 branch April 6, 2023 18:48
@WendellAdriel
Copy link
Contributor Author

Thanks!

I’m happy to help!!! 😉 💪

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.

3 participants