On Tue, Sep 24, 2013 at 04:55:25PM +0100, Lars Wirzenius wrote:
Tue, Sep 24, 2013 at 03:19:28PM +0100, Richard Maw wrote:
> This creates an object that the with statement can use to handle the
> context and clean up the workspace if the body raises an exception.
>
> This is roughly equivalent to having a function that takes a callback of
> what to do while the branch is being initialized, but with less
> boilerplate at the call site.
>
> contextlib is used to create a context manager from a generator
> function. This is less verbose than defining a class with __enter__
> and __exit__ methods.
Terseness doesn't trump clarity. I'd be happier with a more
traditional context manager class for this. That class would then have
its own unit tests, so we have some understanding what it's supposed
to do.
I'm happy to define it in another module and write unit tests, but I find
defining it as a function with the decorator easier to read, since it's
clear what order things are done in and I don't have to worry about the
difference between constructing it and entering it.
> + # TODO: Move this somewhere nicer
This is a code smell of a job half-done. :)
Yeah, I left it in because I wasn't sure of it, and I can't decide
this alone.
> + if not self._checkout_has_systems(gd):
> + raise BranchRootHasNoSystemsError(base_ref)
This makes it impossible to create an empty root repository, which is
a policy decision that should not be embedded deep into the call stack.
It'd be better moved outside the context manager.
Good point, I'll do so.
I'm also not happy about having so many arguments to the method.
It
means the caller needs to pass in a lot of context for the helper to
work. That tends to mean the method interface gets difficult to use
and hard to understand. Five is not too bad, but it's getting the
beginnings of a smell.
I'm thus not sure this context manager is, in fact, a good idea.
It
does put shared code into one place, but that's not always better than
repeating code, if the interfaces etc get convoluted. I think we're
on the edge of that here.
I'd prefer to put more effort into the interface design than write the
code out in 3 different places, since I find the repetition to be a
more odious code smell.