[MERGE] New reconfigure command

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:

>> +    @staticmethod
>> +    def to_checkout(bzrdir, bound_location=None):
>> +        reconfiguration = Reconfigure(bzrdir, bound_location)
>> +        reconfiguration.select_changes(tree=True, branch=True, bound=True)
>> +        if not reconfiguration.planned_changes():
>> +            raise errors.AlreadyCheckout(bzrdir)
>> +        return reconfiguration
> 
> I'd like some docstrings for these functions explaining that a
> Reconfigure object is returned and enumerating the Exceptions raised
> otherwise.

I prefer my code to be self-documenting, and I think this is.  It's
short, the name says what it does.  But if it's required to pass review,
I'll change it.

>> +    def select_changes(self, tree, branch, bound):
>> +        """Determine which changes are needed to assume the configuration"""
> 
> I'd like to suggest that tree and branch be named want_tree and
> want_branch

Okay.  want_bound also?

> I'd prefer the name changes_are_needed() to stress the boolean nature.
> "planned_changes()" sounds like it ought to return the set of planned
> changes. Suggest making this method private as well (though don't mind
> if you think otherwise given it's parameterless).
> 
>> +    def apply(self, force=False):
> 
> Needs a doc string.

I really don't know what the docstring should say.  "Appy the
reconfiguration?"  Why?

> Practically this might always be safe? Technically, if create_branch is
> True and destroy_reference isn't, *reference_info won't be set will it?

Sure, but that's a state you'll never get to, because select_changes
will raise ReconfigurationNotSupported if there's no branch and no
branch reference.  Should I make all these variables private to prevent
people setting them?

> I think this needs a little more doc for the help and reference manual.
> In particular, the doc needs to be clearer on:
> 
> 1. What the default target_type will be if none is given on the command
>    line.
> 2. The rules used to bind a checkout if --bind-to is not given.

Okay.

> I'd like to see an else block trapping and reporting an unknown target
> type. Without it, reconfigure.apply can be called without the
> reconfiguration object being set.

Okay.


>> +    def destroy_branch(self):
>> +        raise NotImplementedError(self.destroy_branch)
>> +
> 
> One line doc string please.

What should it say?  All I can think of is "Destroy the branch", and
it's silly to write a docstring that reiterates the method name.

>> +    def destroy_branch(self):
>> +        """See BzrDir.destroy_workingtree."""
>> +        raise errors.UnsupportedOperation(self.destroy_branch, self)
>> +
> 
> No, see BzrDir.destroy_branch. Well, once the one line doc string is
> there. :-)

You see why unnecessary documentation is bad?

>> +class UncommittedChanges(BzrError):
>> +
>> +    _fmt = 'Working tree "%(display_url)s" has uncommitted changes.'
>> +
>> +    def __init__(self, tree):
>> +        import bzrlib.urlutils as urlutils
>> +        display_url = urlutils.unescape_for_display(
>> +            tree.bzrdir.root_transport.base, 'ascii')
>> +        BzrError.__init__(self, tree=tree, display_url=display_url)
> 
> Is there any benefit to passing the tree parameter to __init__ here?

Yes.  It provides context to any code that catches the exception.

> It's not used in the message.

Printing an error isn't the only way of handling an exception.  You
might want to find out what uncommitted changes there were, for example.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFG6onE0F+nu1YWqI0RAim1AJ4wK/6h6rNTnrWQH83u4jE4BlbYTACeMXAF
E6Fz3K1q2ZS88BJwJ/7UXGc=
=vfUb
-----END PGP SIGNATURE-----