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

Rewrite CLI options handling and output #5

Closed
wants to merge 10 commits into from

Conversation

amrali
Copy link

@amrali amrali commented Dec 23, 2013

I needed more stable and extensible option handling so argparse replaces the custom option handling code. And I also replaced the various print and sys.stderr.write calls with the logging module.

* Use argparse for option handling
* Remove global variables and put them under an argparse namespace
* Replace 'print' statements with the appropriate log calls
* Replace sys.stderr calls with the appropriate log calls
* Remove global variable HUPTIME_DEBUG
@amscanne
Copy link
Owner

Thanks, this is great!

There seem to be a couple of small issues at the moment, though:

  • When running a simple example, I get:

    Traceback (most recent call last):
    File "bin/huptime", line 231, in
    ENV["HUPTIME_UNLINK"] = ARGS.unlink
    File "/usr/lib/python2.7/os.py", line 471, in setitem
    putenv(key, item)
    TypeError: must be string, not None

I assume this is because we need to always cast ARGS.unlink as well?

  • The behaviour for --multi is likely now broken.

This was weird to start with. I think now HUPTIME_MULTI will contain a number, whereas it needs to contain "true". The python script itself is the only thing that uses the N to spawn the appropriate number of child processes.

  • I'd prefer not to support short arguments.

This is a personal preference, but I think there's a good reason. Because I expect the usage of the tool to be automated more than interactive (i.e. it'll end up in init scripts or configuration files), I'd rather force the long form arguments. In my experience, using the long form arguments in scripts is best practice simply because it makes things more readable and maintainable for others (just compare "huptime --unlink /var/run/myprog.pid --revive --fork {...}" to "huptime -u /var/run/myprog.pid -r -f {...}").

@amrali
Copy link
Author

amrali commented Dec 23, 2013

Will eliminate support for short arguments, I do share your preference as well just thought it might appeal to a wider audience that way. (except for --debug/-d perhaps? since this is more interactive?)

RE: --unlink broken
Will fix.

RE: Behavior of --multi
So you are saying that the environment variable should contain either "true" or "false" (depending on whether --multi > 1) ?

@amrali
Copy link
Author

amrali commented Dec 23, 2013

Fixed the above (--debug/-d stays, let me know if you do not want that), rebased on master.

This will automatically clean-up all the child processes spawned
by --multi whenever the parent dies. This is much more useful for
integration with a supervisor (like upstart) because you can easily
kill the collection without needing to use killall or --stop.
@amscanne
Copy link
Owner

I added a commit to touch it up a bit (print help w/ no command, etc.) and made #16. Anything missing from there?

@amrali
Copy link
Author

amrali commented Dec 26, 2013

#16 looks good

@amscanne
Copy link
Owner

Rolled in #16 (where uh, it continues to languish in limbo. Sorry.)

@amscanne amscanne closed this Jan 10, 2015
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.

4 participants