-
Notifications
You must be signed in to change notification settings - Fork 305
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
[stdout stream] Added an option to stream outputs #39
Conversation
0f64ffb
to
b4f5d8d
Compare
parser.add_option('--notify_error', action='store', type='string', default='', | ||
help="a message string and @people directives to send notifications in \ | ||
case of error.") | ||
parser.add_option('-f', '--flush_live', action='store_true', dest='flush_live', default=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make it True
by default ? I think we might want to use this option more often than we won't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was wondering as well but so far the behaviour was to have it set to false and I thought maybe some of our clients use dogwrap in some scripts of their owns. So basically I did that for "backward compatibility" but since dogwrap has been released very recently maybe we can put it to True by default. Shall I do that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think 'flushing live' versus 'once the command is completed', can introduce backward compatibility.
Did you have a chance to test with Python 2.6 and Python >= 3.3 ? |
b4f5d8d
to
a03b226
Compare
a03b226
to
7f9764c
Compare
Quick remark about the command line option: |
I can't think about any reason why we would rather have the output provided once the command is completed (versus 'real-time'). Should we just drop it and avoid an extra option ? |
Yeah, I dont see either to be honest. Maybe we can just enable it by default and add a -b option for (--buffer-outputs) so that we get rid of this letter issue at the same time ! Also maybe adding a --mute or --silent option to prevent the output from being printed may be a good idea even though it's also almost as easy to just redirect the output of dogwrap to /dev/null. What do you guys think ? |
I don't really see any use case for the -b option but it doesn't hurt to keep it "just in case". |
I like the idea of |
👌 |
Thanks for doing this—I'm super happy to have realtime output! 👍 Generally, I'm for less flags until we see an actual use case for them. If we can reliably provide realtime output, I don't see any reason not to. But so long as it isn't much extra code, I guess it's nice to leave the I also don't see much case for |
9347987
to
0f204b6
Compare
Ok Doug, I just got rid of the silent option. PR should now be ready to be merged. |
- By default, dogwrap now forwards the stdout and stderr of the command it executes to the terminal launching dogwrap. - This behaviour can be prevented using the --buffer_outs option (dogwrap then acts just as it used to do) - I also added some comments in the code. - Finally, I added help messages for the different possible options, as well as an "USAGE" description and and version number (automatically enabled --version option for dogwrap).
0f204b6
to
3491e3e
Compare
[stdout stream] Added an option to stream outputs
it executes to the terminal launching dogwrap.
(dogwrap then acts just as it used to do)
well as an "USAGE" description and and version number (automatically
enabled --version option for dogwrap).