[MERGE] bzr rm should delete the working file (Bug #82602)

[Marius Kruger]

On 4/12/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
>
> John Arbash Meinel has voted +0.
> Status is now: Waiting
> Comment:
> This has come quite a ways, and I would really like to see it in 0.16.


me too


If you need some help, just ask, and I can try to do some of the things
> I mention here


I'll try my best to get it done..


I think this should have a NOTES WHEN UPGRADING: section in NEWS.
> Because it means that 'bzr rm' will change how it works.
>
> I know there have been mention of people who are already using 'bzr rm'
> and expecting it to leave the file alone (James Troup mentioned this for
> versioning files in /etc). So we need to make sure to convey this change
> well. (We aren't destroying the data, you can 'bzr revert' or 'bzr rm
> --keep', but people need to know about it).


Should I just put the whole thing in a the NOTES WHEN UPGRADING section ?


I don't quite understand this change:
> -    only 'added' files will be removed.  If you specify both, then new
> files
> -    in the specified directories will be removed.  If the directories
> are
> +    only 'added' files will be removed.  If you specify both, then new
> +    files in the specified directories will be removed.  If the
> directories are
>
> It seems you re-wrapped the line, only to cause the next line to become
> *too* long.


[DONE]


I'm not sure about making these take multiple paths:
>       def failUnlessExists(self, path):
> -        """Fail unless path, which may be abs or relative, exists."""
> -        self.failUnless(osutils.lexists(path),path+" does not exist")
> +        """Fail unless path or paths, which may be abs or relative,
> exist."""
> +        if not isinstance(path, basestring):
> +            for p in path:
> +                self.failUnlessExists(p)
> +        else:
> +            self.failUnless(osutils.lexists(path),path+" does not
> exist")
>
> I think we generally avoid apis that use 'isinstance'. I'm probably
> about 75% okay on this, though. Since it is meant as a way to make
> writing tests easier, and as long as the tests are easier to write and
> easier to understand, it seems good.


I'm leaving as is, unless someone is less than 50% ok with it.. :)


I prefer 'self.run_bzr' to 'self.runbzr', but that is more of a
> guideline than a requirement.


ok I'll convert to run_bzr_captured,
but 'self.runbzr' seemed more suited to lazy people like me...
[DONE]

* these tests follow this pattern:
        (out, err) = self.run_bzr_captured(['remove', '--keep', 'a'])
        self.assertEquals(out.strip(), "")
        self.assertEquals(err.strip(), "removed a")
which we might want to do better:
        self.run_bzr_captured(['remove', '--keep', 'a'], out_regex='',
err_regex="removed a")
also can't I make it like runbzr, to auto expand a sting?
  (your probably gona say no as you don't like isinstance magic :(


PEP8 says that:
> (out,err) = self.runbzr(
> should be
> (out, err) = self.


[DONE]

You added a test class to test_workingtree.py, but it should probably be
> a test in workingtree_implementations/test_remove.py
>
> +
> +class TestRemove(TestCaseWithTransport):
> +    """Tests WorkingTree.remove"""
>
> It needs to be written slightly differently, first by inheriting from
> TestCaseWithWorkingTree.
>
> You can look at other 'workingtree_implementations' tests to get a
> reference for how to do it.
>
> Putting it in bzrlib/tests/test_workingtree.py means it will be tested
> with our current default WT format. Putting it in
> workingtree_implementations means it will get tested against all of our
> WT implementations (currently WT2, WT3, WT4). The biggest benefit is
> that it adds tests for any new implementations, to make sure we maintain
> interface consistency.


[DONE]. all tests passed first time!


I don't see why you needed to do this:
> -                if not tree.has_id(file_id):
> +                if not tree.has_id(file_id) or not file_id in
> tree.inventory:
>
> If you found a case where "tree.has_id(file_id)" was returning True, but
> file_id not in tree.inventory, then you found a bug. And it should be
> fixed at the 'has_id()' layer.


...mm thats weird.
I had to do this to make my tests pass, but I undid it now, and it works
fine..
I think at the time I did it, I used self.changes_from(...,
specific_files=...),
wrong, since I didn't dereference them...

[REMOVED]

...
I'm still busy with the rest, but will get it done asap.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20070413/ab7505b9/attachment-0001.htm