[Merge] lp:~stgraber/upstart/upstart-initctl2dot-python3 into lp:upstart

[Stéphane Graber]

> Just a couple more suggestions, since why not.
> 
> * No parens are needed in `from subprocess import Popen, PIPE`

Done

> * How about switching to argparse instead of optparse?

Done

> * In header(), the global statement isn't necessary because you're not
> assigning to options

Done

> * I'd probably rewrite the concats in header() into a triple-quoted multiline
> string, with {options.foo} replacements

Done

> * Remove the global statement from footer()

Done

> * There should be some better way to get rid of the multiple concats in
> footer() too

Done. Pre-processed the ifs, then used a giant string with format, similar to header()

> * 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

Kept it as-is for now as I'm not a fan of using regexps when not absolutely necessary :)

> * Why does mk_node_name() even exist? ;)

No good reason apparently, dropped.

> * show_events(): remove global and rewrite the concats

Done.

> * Remove the globals from show_events()

Done

> * Remove the global in show_jobs()

Done

> * Remove the globals in show_jobs()

Done

> * In show_jobs(), the `if not restrictions_list: return` is kind of
> unnecessary.

Right, at first read, I assumed that restrictions_list could be None, but rechecking the code, that's not actually the case, so iterating will always work and that if statement is unnecessary. Dropped.

> * 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)

Done

> * read_data(): `for line in ifh:` is probably good enough

Agreed, done.

> * 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

I'd rather not depend on python 3.3. Some people may still want to use the fixed script on something slightly older, but that's good to know nevertheless.

> * line 430: extra parens

Fixed that one and another one a few lines further down.

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


As far as I can tell, the script still works with those changes and the output is similar.
-- 
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.