[PATCH] Re: Feature Request: 'bzr mv --after' to tell bzr that file(s) have already been moved in the working tree

[Hanns-Steffen Eichenberg]

On 11/8/06, John Arbash Meinel <john at arbash-meinel.com> wrote:
>
> Hanns-Steffen Eichenberg wrote:
> > Hello,
> >
> >
> > This patch is still a draft, but should be better then the first
> version.
> >
>
> It is starting to look pretty good.


lol. after THAT much help it would be very embarrassing if it would not have
improved ;-)


> For parameters like 'after' I usually prefer to see:
>
> tree.rename_one(rel_names[0], rel_names[1], after=after)
>                                             ^^^-named parameter.


I just have to get used to named parameters.

> - have read PEP 8  :-)
> > - removed an unnessecary API change
> > - deprecated one API
> > - replaced the dictionaries that i called 'tuples' with a ValueObject
> > - fixed some bugs
> > - made the code a little bit more robust (i hope so)
> > - written some new blackbox tests for mv
> > - switched from pure Win32 to cygwin for development (feels more
> > comfortable)
> >
>
> Interesting. It probably is a little bit more of a match. I always
> avoided it because it runs about 50% slower. At least on my machine
> cygwin adds a lot of overhead.


Me typing 'll' or 'export' into a DOS box is more of a slow down.

>
> > what i have not done:
> > - run the complete testsuite. i still have problems running the
> > selftest. I need to spend some time on it.
> >
> >
> > Again, thank you for the first review. I hope this version is an
> > improvement.
> >
> >
> > Ciao,
> >   Steffen
> >
>
> For tests, we are trying to simplify them, so that each test only is
> testing 1 thing, rather than testing several things in each test. It's
> something that I'm not always good at, but I'm learning from Robert. So
> I would recommend a few things here:


I will rewrite the tests.


>
> v- A doc string or comment here indicating this is simulating where the
> user does:
>
> "mv a b; touch a; bzr mv a b" might be helpful here.


will do


We should also have a test that to_name is deprecated but still works.
> That would probably go into bzrlib/test_workingtree.py. You can use
> self.callDeprecated() to catch the warning, and make sure the right
> warning is issued, and then check the function still has the right effect.


will try

...
>
> +    class _RenameEntry(object):
> +        def __init__(self, from_rel, from_id, from_tail, from_parent_id,
> +                     to_rel, to_tail, to_parent_id):
> +            self.from_rel = from_rel
> +            self.from_id = from_id
> +            self.from_tail = from_tail
> +            self.from_parent_id = from_parent_id
> +            self.to_rel = to_rel
> +            self.to_tail = to_tail
> +            self.to_parent_id = to_parent_id
> +            self.only_change_inv = False
> +
> +        def set_only_change_inv(self, only_change_inv):
> +            self.only_change_inv = only_change_inv
> +
>
> I think it is better if _RenameEntry is just a private class at module
> scope, rather than a nested class.
>
> Or at the very least, I would define a nested class at the start of the
> encompassing scope, rather than in the middle.


I would like to have a stronger statement here. What is the desired code
style for bzr?
  - Is it OK to use nested classes or not?
  - where *exactly* should nested or private classes be placed in the
source?

I ask, because in most projects i encountered the rules 'keep related things
together,
because it is easier to read' and 'define elements in the scope you are
using them'.
If you want to get an overview of all elements, use the functionality of
your favorite IDE.

Please do not get me wrong. I do not want to question the code style for
bzr, i want
to understand and learn it.


Oh, and I would like to see the error messages unified a little bit
> more. I really don't see why we need move() and rename_one() to be
> separate functions. I saw that 'rename_one' seems to raise:
> "can't determine destination directory id for"


I did not dare to refactor the existing error messages. If it is OK, i
will be glad to that.

I do not completely understand your statement: do you just want to
unify the error messages of 'move()' and 'rename_one()' or do you like
to see both methods being refactored into one?


Also, the check:
>
> if to_dir_id is None and to_dir != '':
>
> should probably be:
>
> if to_dir_id is None:
>
>
> At one point, the root of a tree didn't have a file id, but it should
> now, so we don' need to special case "to_dir = ''".



will fix that.

I can not promise to get those things ready this week.

Anything else that can / should be done with the mv command and its
tests?

Ciao,
  Steffen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20061108/9351dfef/attachment.htm