Change strategy of output.

JohnSundell / ShellOut

Easily run shell commands from a Swift script or command line tool

MIT License

872 stars 85 forks source link

Change strategy of output. #3

Closed yageek closed 7 years ago

yageek commented 7 years ago

Use a different strategy to retrieve the content of STDOUT and STDERR. The ShellOutError structure has been changed but the API remains the same. The structure now contains both STDOUT and STDERR as Data and we can retrieve the equivalent String using the previous properties message and output. From my memories, any non 0 terminationStatus should be considered as an error. This also allows to introduce a parameter printOutput which will output the received data to STDOUT and STDERR.

JohnSundell commented 7 years ago

Hi @yageek! Thanks for this PR! Could you please outline what the target usecase is for this change - that is - why is it better than the current implementation? 🙂

yageek commented 7 years ago

The thing I wanted to add is still not working after some tests (last bullet) :(. The elements I wanted to improve:

[X] Any non 0 output should be considered as an error, not only when something has been written to STDERR.
[X] Possibility to retrieve any Data and the termination status when an ShellOutError is thrown.
[X] As soon as characters are received by the Process, I would like to stream them directly out instead of waiting the completion to finish. In a CI script, it could be useful to stream the received elements as long as the compilation is going forward.

I am investigating a little further about how to stream the output correctly. Is there some slack channel to discuss about improvement and features but the way?

yageek commented 7 years ago

After taking a look more closely, this is working in deed. I have a created a compilation script that behaves like travis and I am able to see the output as it is streamed. This behaviour could be used also for marathon run.

JohnSundell commented 7 years ago

@yageek Thanks again for this, I love the idea - it's going to make a big improvement to Marathon, since you don't have to wait for the full output 👍 I put some comments inline, and also some tests are needed before this can be merged in 🙂

yageek commented 7 years ago

I have updated the API and added the missing tests. The only question remaining is to allow/disallow access to the raw Data in case of error.

yageek commented 7 years ago

I renamed the arguments. Your proposition made more sens. About the error, according to me, we should give the opportunity to have access to the data. Here when we are converting to Stringwe assume we are using an utf8 encoding. May be some programs outputs text in a different encoding. Providing an access to the internal buffers will allow any transformation.