yellow team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

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

I'm not seeing it, but I'll take your word for it.  Oh!  I think you
mean that the parameter is used, right I see that; I mean that the
*argument* is never used, i.e., no caller of bzr_whois ever passes an
argument for "parser", therefore it can be removed and its use in the
body of bzr_whois can be replaced with parseaddr.

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

The function can still return a callable so you get those same benefits.
The transformation would be to remove the decorator and change the yield
into a return.  Then instead of doing a "with ssh(...) as foo" you'd do
a "foo = ssh(...)".

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

"handle" is certainly reasonable.

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

Well, I guess I learned something new today. :)

-- 
https://code.launchpad.net/~yellow/launchpad/lxcsetup/+merge/89660
Your team Launchpad Yellow Squad is subscribed to branch lp:~yellow/launchpad/lxcsetup.