[Merge] lp:~stgraber/upstart/upstart-initctl2dot-python3 into lp:upstart
Barry Warsaw
barry at canonical.com
Thu Nov 29 12:13:22 UTC 2012
Review: Needs Fixing
Just a couple more suggestions, since why not.
* No parens are needed in `from subprocess import Popen, PIPE`
* How about switching to argparse instead of optparse?
* In header(), the global statement isn't necessary because you're not assigning to options
* I'd probably rewrite the concats in header() into a triple-quoted multiline string, with {options.foo} replacements
* Remove the global statement from footer()
* There should be some better way to get rid of the multiple concats in footer() too
* sanitize() seems pretty inefficient. maybe that doesn't matter for this script, but it might be better written with a re.sub() where `repl` is a function that knows the mappings
* Why does mk_node_name() even exist? ;)
* show_events(): remove global and rewrite the concats
* Remove the globals from show_events()
* Remove the global in show_jobs()
* Remove the globals in show_jobs()
* In show_jobs(), the `if not restrictions_list: return` is kind of unnecessary.
* None of the show_*() methods need their globals either (just keep at it for the rest of this file ;) - you only need globals if you're *assigning* to a global variable (not if just referencing it, or calling methods like .append() on it)
* read_data(): `for line in ifh:` is probably good enough
* Might want to use a context manager to handle ifh in read_data(). For Python 3.3, see http://docs.python.org/3/library/contextlib.html#contextlib.ExitStack
* line 430: extra parens
Probably other stuff to be cleaned up, but those are the major things that jump out at me. I can take a crack at it if you want.
--
https://code.launchpad.net/~stgraber/upstart/upstart-initctl2dot-python3/+merge/136721
Your team Upstart Reviewers is requested to review the proposed merge of lp:~stgraber/upstart/upstart-initctl2dot-python3 into lp:upstart.
More information about the upstart-devel
mailing list