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

[stdout stream] Added an option to stream outputs #39

Merged
merged 1 commit into from
Apr 24, 2015

Conversation

elafarge
Copy link
Contributor

  • 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).

@elafarge elafarge force-pushed the etienne/dogwrap-nobuf branch from 0f64ffb to b4f5d8d Compare April 23, 2015 18:55
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,
Copy link
Member

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.

Copy link
Contributor Author

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 ?

Copy link
Member

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.

@yannmh
Copy link
Member

yannmh commented Apr 23, 2015

Did you have a chance to test with Python 2.6 and Python >= 3.3 ?

@elafarge elafarge force-pushed the etienne/dogwrap-nobuf branch from b4f5d8d to a03b226 Compare April 23, 2015 20:58
@yannmh
Copy link
Member

yannmh commented Apr 23, 2015

This fix #33
Thanks @elafarge !

cc @ddaniels888

@elafarge elafarge force-pushed the etienne/dogwrap-nobuf branch from a03b226 to 7f9764c Compare April 23, 2015 22:25
@degemer
Copy link
Member

degemer commented Apr 24, 2015

Quick remark about the command line option:
-f for most Linux commands mean --force, so you might want to choose another letter (-d for --debug maybe if it's optional?) to avoid confusion.

@yannmh
Copy link
Member

yannmh commented Apr 24, 2015

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 ?
@ddaniels888 any thoughts ?

@elafarge
Copy link
Contributor Author

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 ?

@elafarge
Copy link
Contributor Author

I don't really see any use case for the -b option but it doesn't hurt to keep it "just in case".
But I think many users would be glad to use dogwrap in shell scripts, and therefore they would probably prefer it to be silent ?

@yannmh
Copy link
Member

yannmh commented Apr 24, 2015

I like the idea of --silent and -b.

@yannmh
Copy link
Member

yannmh commented Apr 24, 2015

👌

@ddaniels
Copy link
Contributor

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 --buffer option in case someone was relying on that (for reasons I don't know).

I also don't see much case for --silent. I think just doing the standard pipe to /dev/null if you want that is easier, and requires no extra code to test/maintain.

@elafarge elafarge force-pushed the etienne/dogwrap-nobuf branch from 9347987 to 0f204b6 Compare April 24, 2015 16:57
@elafarge
Copy link
Contributor Author

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).
@elafarge elafarge force-pushed the etienne/dogwrap-nobuf branch from 0f204b6 to 3491e3e Compare April 24, 2015 17:34
yannmh added a commit that referenced this pull request Apr 24, 2015
[stdout stream] Added an option to stream outputs
@yannmh yannmh merged commit c6ce25c into master Apr 24, 2015
@elafarge elafarge deleted the etienne/dogwrap-nobuf branch April 24, 2015 18:23
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

4 participants