Live output option for rspec command #710
Conversation
* Add new option live_output * If passed, show output in real time, instead of waiting for the command to finish
sds
left a comment
sds
left a comment
There was a problem hiding this comment.
Thanks for the pull request!
Have you tested this locally? What did you observe in the case where multiple hooks were outputting in parallel? The interleaving of output could get very messy, and unless we start to dictate that live output mode also simultaneously disabled parallel hook runs, this will not be a great user experience.
The approach being taken here will suffice for those who really want the live output functionality for the specific use case it enables—pre-push hooks that don't need to read the standard output/error streams to determine the outcome of the hook run, just the process exit status. However, not all users will be using Overcommit in that way, so we will not be merging this as-is.
Tremendously appreciative of you taking an initial stab at adding support for this functionality, but if we're going to do this, some explicit thought needs to be given as to how to make this a workable experience for the Overcommit platform, not just some specific hooks.
| end | ||
|
|
||
| def build_result(process, out, err, options = {}) | ||
| return Result.new(process.exit_code, '', '') if options[:live_output] |
There was a problem hiding this comment.
This can potentially break any processing done by the hook. If the exit_code is non-zero, hooks can sometimes read the stderr/stdout to determine the kind of failure that occurred. Given all the different ways Overcommit hooks could be used, we can't introduce a change that fundamentally breaks the behavior.
2 participants
fixes #583