> 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.
--
https://code.launchpad.net/~yellow/launchpad/lxcsetup/+merge/89660
Your team Launchpad Yellow Squad is subscribed to branch
lp:~yellow/launchpad/lxcsetup.
--
Mailing list: https://launchpad.net/~yellow
Post to : [email protected]
Unsubscribe : https://launchpad.net/~yellow
More help : https://help.launchpad.net/ListHelp
