Code review comment for lp:~yellow/launchpad/lxcsetup

> The script looks great, don't let the wall of text below fool you. :)

Hooray!
My thoughts on some of your observations:

> We can use email.Utils.parseaddr instead of _parse_whoami.

I agree, I didn't think about looking at the standard library :-(

> I assume the "parser" argument for bzr_whois was intended as a
> dependency injection point, but since it's not used it can be removed.

It is, see line 127 of the diff.

> Since "ssh" doesn't perform any cleanup actions (i.e., there is no code
> after the yield), it could just be a regular function.

Yes, my same doubt. I ended up using a context manager just to separate connection arguments from the real "call" argument, and to avoid repeating username and location on each ssh call.

> I don't understand the meaning of "clean" in _clean_users,
> _clean_userdata, and _clean_ssh_keys. Maybe it's an assertion that they
> are clean, in that case I'd expect the functions to be named
> "validate...", but they do more than validation, so that doesn't sounnd
> right either. Maybe "set" would be the right word ("set_directories",
> etc.).

Argh... naming things... I used "clean" as e verb, because after calling them we can assume the namespace to be "clean", usable. Maybe "handle_*" could be better?

> If a function_args_map action raises an exception in main, the exception
> is returned (not reraised) and the return value of main is passed to
> sys.exit which expects an integer. That doesn't seem quite right.

sys.exit expects other types too, and in that case it prints the string representation of the passed object to stderr and then exits with 1, that's what we want.

Thank you Benji for all your suggestions.