[Merge] lp:~jamesodhunt/upstart/initctl2dot-fix-for-subdirs into lp:upstart

[Dmitrijs Ledkovs]

On 22 March 2013 12:37, James Hunt <james.hunt at canonical.com> wrote:
>  # Map dash to underscore since graphviz node names cannot
> -# contain dashes. Also remove dollars and colons
> +# contain dashes. Also remove dollars and colons and replace other
> +# punctuation with graphiviz-safe names.
>  def sanitise(s):
>      return s.replace('-', '_').replace('$', 'dollar_') \
>              .replace('[', 'lbracket').replace(']', 'rbracket') \
>              .replace('!', 'bang').replace(':', 'colon').replace('*', 'star') \
> -            .replace('?', 'question').replace('.', '_')
> +            .replace('?', 'question').replace('.', '_').replace('/', '_')
>

I wonder if we can use translate instead here:
sanitize_table = str.maketrans({
    '-': '_',
    '$': 'dollar_',
    '[': 'lbracker',
    ']': 'rbracker',
    '!': 'bang',
    ':': 'colon',
    '*': 'star',
    '?': 'question',
    '.': '_',
    '/': '_',
})
return s.translate(sanitize_table)

Somehow this is easier to read & edit =) maybe make that translation
table global, such that we don't create that object each time
sanitized is called.

>
> +    parser.add_argument("--user",
> +                        dest="user",
> +                        action='store_true',
> +                        help="Connect to Upstart user session (default if running within a user session).")
> +
> +    parser.add_argument("--system",
> +                        dest="system",
> +                        action='store_true',
> +                        help="Connect to Upstart system session.")
> +
>      parser.set_defaults(color_emits=default_color_emits,
>                          color_start_on=default_color_start_on,
>                          color_stop_on=default_color_stop_on,
> @@ -504,6 +535,25 @@
>      if options.restrictions:
>          restrictions_list = options.restrictions.split(",")
>
> +    upstart_session = os.environ.get('UPSTART_SESSION')
> +

I'd change it to be os.environ.get('UPSTART_SESSION', False), such
that we get explicit False, instead of vague None when it's not
defined.

> +    use_system = True
> +
> +    if options.system == False and options.user == False:
> +        if upstart_session:
> +            use_system = False
> +        else:
> +            use_system = True
> +    elif options.system == True:
> +        use_system = True
> +    elif options.user == True:
> +        use_system = False
> +

"if foo == True" is not pythonic, simply use "if foo:"

Further more you are using if comparison to derive and assign a bollean value.
Why not just assign it?

use_system = options.system or not options.user or not upstart_session

Or you can make it easier by defining the option --user to be
dest='use_system', action='store_false' & --system to be
dest='use_system', action='store_true' then you will only need to test
for the environment variable.
(It will give less flexibility in the future, but I don't think these
two options will evolve beyond simple binary choice).

Regards,

Dmitrijs.

-- 
https://code.launchpad.net/~jamesodhunt/upstart/initctl2dot-fix-for-subdirs/+merge/154935
Your team Upstart Reviewers is requested to review the proposed merge of lp:~jamesodhunt/upstart/initctl2dot-fix-for-subdirs into lp:upstart.