Code review comment for lp:~frankban/launchpad/lpsetup-initial

Revision history for this message
Robert Collins (lifeless) wrote :

Hi, a few points for you to consider:
 - this might be a lot better off in the lp-dev-tools project, its
clearly not part of LP itself. That would also make it easier to split
it into N files as you would not be stuck in the bootstrap stage of
the users environment (we could package that and mandate the package
be installed).
 - the environ context manager is duplicative of the
EnvironmentVariable fixture; it would be nice not to rewrite code that
has already been written and tested (and is used in our environment
too!) Yes, I know that this is small, but it does all add up, and
someone somewhere will eventually be cursing that they have to redo
this / fix a unicode escaping issue in the code or whatever.

 - I'm not fussed about reusing commandant or the bzrlib UI framework;
if I was writing something new that needed subcommands I would do it
differently than I did in bzrlib (and in fact I have since - with a
leaner, faster, more testable and machine drivable system in testr).
If you wanted to reuse that, its probably not quite there for reuse,
but we could split it into framework-and-user and make it so.

HTH,
Rob

« Back to merge proposal