'bzr rm' leaves directories behind?

[Marius Kruger]

thank you Robert for the review.
see my comments below

On 6/19/07, Robert Collins <robertc at robertcollins.net> wrote:
> Hi, I hadn't reviewed your previous patches. I've had a quick look at
> this and have a few comments...
>
> self.run_bzr("commit", "-m", "empty")
> is preferred over
> self.run_bzr("commit -m empty")

There were a lot of discussion about this for [MERGE] bzr rm should delete
the working file (Bug #82602)
and at least Martin sort of agreed with me that in principle:

> *me: * in general don't you think 'bzr rm xyz' is nicer than
> ['bzr','remove,'xyz']?
> *Martin: *i do
>
But in any case this patch is not trying to change things either way, It
just keeps using
the same syntax which is already used throughout the file.
This being a bug fix, I'd rather not try to sort out what is preferred usage
of run_bzr now.

>
> In fact run_bzr("string with spaces") should not work at all.

It does work because for this test case run_bzr_captured is overwritten to
allow this. My intention was to get this functionality into the super class,

but when this code landed, that was considered too risky.

Because of the method
>
>
> +    def _assertRemoveErrorContainsRe(self, e, file_detail_re):
>
> According to the comment this is a workaround for AssertContainsRe not
> supporting DOT_ALL. Is there some reason you didn't just fix/enhance
> AssertContainsRe?

I am usually that sort of developer which fixes everything there and then,
but reviewers complain to me that I 'fix' too many unrelated things in one
patch, which made reviewing difficult.
However I think this patch should rather go in than sooner rather than
later,
so I'll commit to enhance AssertContainsRe in a follow up patch if you like.

>
> Other than that it looks ok to me.
> +1 conditional on the above things being corrected/discussed.

hope this is satisfactory,
and thanks again for the review, as I was starting to think you guys are not
speaking to me for
introducing this bug in the first place.

regards
marius


-- 
bazaar-vcs.org
Because I don't trust Version Control Systems with less than 6350 unit
tests.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20070621/0a79974b/attachment.htm