[merge][rfc] New command: 'bzr remove-tree'

[Daniel Silverstone]

On Fri, 2006-11-10 at 17:20 -0600, John Arbash Meinel wrote:
> I think the command is okay. We can start it off as hidden, and then
> make it unhidden later if there is a lot of requests for it.
> To make a function hidden, you do:
[snip]

Done, thanks.

> It looks like you copied some other tests which were written in an older
> style. So I'll have quite a few points. It isn't strictly wrong, but we
> are trying to write tests which are clearer to understand.

That's true, I copied tests/blackbox/test_cat.py

> v- I think we avoid -*- lines, especially since we avoid adding
> non-ascii characters into the source code.

Removed, Also updated the copyright to 2006 :-)

> > +# Copyright (C) 2005 Canonical Ltd
> > +# -*- coding: utf-8 -*-
[snip]
> v- If you only have 1 line, we try to put the closing """ on the same line.
Done
> > +"""Black-box tests for bzr remove-tree.
> > +"""
> > +
> > +import os
> > +
> > +from bzrlib.tests.blackbox import ExternalBase
> -- two spaces between module scope definitions (so you need another
> blank space here)
Done
> > +
> > +class TestRemoveTree(ExternalBase):
> > +
> 
> v- We have the helper functions "self.failIfExists()" and
> "self.failUnlessExists()" for this sort of thing.

Gotcha, replaced.

> ^- We've switched to setting up trees and branches using the bzrlib api,
> and then only call run_bzr() for the thing we are actually testing. So
> instead this should be:

Okay

> v- For this, I would probably use 'self.run_bzr_error()' so that you can
> make sure that bzr is failing for the correct reason, and printing a
> reasonable error message to the user.

Thanks.

> ^- Also, you are really doing 4 different tests (original branch, copy
> of branch, heavy checkout, lightweight checkout).
> So it should really be 4 tests. You can use def setUp(self) as a place
> to create the initial tree along with a single commit, and then you have
> 4 relatively short test functions, which make it clear what the
> dependencies are and are not.

Done

> v- 'location' should just be '.' not u'.'.
Took from cmd_revno, changed.
> I also prefer:
> d = bzrdir.BzrDir.open(location)

Done

> I don't think we want 'open_containing()', because it is kind of silly
> to remove a working tree that you are currently deeply nested in. Since
> it will try to remove your current working directory.

Yes that's fair

> And I think always using open_containing() will actually cause problems
> where a user thinks they are removing one tree, but due to a typo, they
> end up removing a different one.

Indeed. Fixed

> For try/except functions, we prefer to catch them in a small area,
> rather than over a lot of functions. So it should be:
[snip]

Taken on board and refactored that function. Thanks

> You also should wrap things at 79 characters.

Oops, sorry, I didn't realise I hadn't

> So there are a few things to be done, but I think you have the general
> idea. Your test coverage is good, it should just be refactored into
> multiple small tests.

Thanks

> I'd be happy to merge a cleaned up patch.

Just doing the last tidying and then I'll post a replacement bundle.

D.

-- 
Daniel Silverstone                         http://www.digital-scurf.org/
PGP mail accepted and encouraged.            Key Id: 2BC8 4016 2068 7895