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

Hanns-Steffen Eichenberg scameronde at googlemail.com
Mon Nov 6 19:05:29 GMT 2006


On 11/6/06, John Arbash Meinel <john at arbash-meinel.com> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> I highly recommend learning to write tests along with code. I'm not 100%
> dedicated to test-driven yet, but test-as-you-go is really nice.
> Test-driven can be really good too, I just don't always use it.
> (Basically, to know your code works, you have to run it anyway, so you
> might as well put that in a real test, and then in the future you will
> be aware if that test ever fails).


I completely aggree with you. I would have liked started with
writing tests, but i did the patch more for myself in the first place.
I will write the tests as soon as possible.


2) We generally submit patches as text files (not .zip, etc) because
> that way we can review them inside an email, rather than downloading it,
> unzipping it, opening another editor, and the copying code chunks back
> to do comments.


will comply next time


3) If you read HACKING, you can see some of our style guides, but the
> important ones are:
>
>    1) Use spaces, not tabs to indent
>    2) Wrap lines at 79 characters


whooops. i thought i  did a '%s/\t/    /g'  before  commit ....

4) You changed the parameter 'to_name' to 'to_dir'. I agree that it is
> more descriptive, but because in python any parameter can be accessed by
> its' name, we have to maintain API compatibility. (eg
> move(from_paths=[], to_name='foo'))
>
> However, we can do this:
>
> def move(self, from_paths, to_dir=None, after=False, **kwargs):
> ...
>
> if to_dir is None:
>   to_name = kwargs.get('to_name', None)
>   if to_name is None:
>     raise ? ('You must supply a target directory')
>   else:
>     symbol_versioning.warn('The parameter to_name was deprecated'
>                            ' in version 0.13. Use to_dir instead',
>                            DeprecationWarning)
>     to_dir = to_name
>
>
> I'm not sure what the exact exception should be. Python would raise an
> TypeError if you didn't supply a required argument. Which seems good
> enough here. It is a programmer error, not a runtime error. (We
> generally avoid raising builtin errors, but in this case it makes sense)


i will fix it.

5) This section:
> +            tuple = {}
> +            tuple['from_rel']  = from_rel
> +            tuple['from_id']   = from_id
> +            tuple['from_tail'] = from_tail
> +            tuple['from_parent_id'] = from_parent_id
> +            tuple['to_rel']    = to_rel
> +            tuple['to_tail']   = from_tail   # from_tail == to_tail
> +            tuple['to_parent_id'] = to_dir_id
> +            from_to_tuples.append(tuple)
>
> First, you call it a 'tuple' but it is a dictionary. (Python has a tuple
> type (1, 2, 3))
>
> Second, PEP8 requests that you don't add whitespace before your '='
> sign. I understand why you like it, but for consistency (and to prevent
> people going back and forth changing this sort of thing) we've decided
> to follow PEP8's recommendations.


What is PEP8 ?

Third, I think you have enough stuff going on here that a real object
> would be better than a dictionary. We won't be creating so many of them
> that a dictionary performance would matter. And I think it ends up being
> clearer. And if you want to keep the naming, you still can, it just
> becomes:
>
> 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.only_change_inv = None
>
> ...
>
>    entry = _RenamEntry(from_rel=from_rel,
>                        from_id=from_id,
>                        from_tail=from_tail,
>                        ...
>                       )


seems like  i have coded too much Ruby and not enough Python lately :-)
Dictionaries instead of ValueObjects are overused in Ruby.
Will change it.

6) All of the mutter() statements should be change:
> +        mutter("  from_id   {%s}" % from_id)
> +        mutter("  from_rel  %r" % from_rel)
> +        mutter("  to_rel    %r" % to_rel)
> +        mutter("  to_dir    %r" % to_dir)
> +        mutter("  to_dir_id {%s}" % to_dir_id)
> +
>
> This wasn't your bug, they were already incorrect. But at the least,
> these should be:
> +        mutter("  from_id   {%s}", from_id)
> +        mutter("  from_rel  %r", from_rel)
> +        mutter("  to_rel    %r", to_rel)
> +        mutter("  to_dir    %r", to_dir)
> +        mutter("  to_dir_id {%s}", to_dir_id)
> +
>
> Otherwise if there was a '%' symbol in one of the filenames, it would
> cause mutter() to puke, because mutter() also does a string expansion
> with the arguments. (Mutter should be updated to not puke, but this is
> still incorrect).
>
> Further, I prefer to see stuff like this in a single call, so something
> like:
>
> mutter('rename one:  {%s}\n'
>        '  from rel:  %r\n'
>        '  to_rel:    %r\n'
>        '  to_dir:    %r\n'
>        '  to_dir_id: {%s}\n',
>        from_id, from_rel, to_rel, to_dir, to_dir_id)


Will change it, too.

7) I also discovered that your bundle used 'dos' line endings (CRLF).
> This is a bug with how we interact with Windows (win defaults to setting
> stdin/stdout/etc as text mode, when we expect binary mode). But it means
> that you should use 'bzr bundle --output=filename'


Can you please update the 'Giving Back' document accordingly?

I hope this isn't more feedback than you wanted. I think the general
> flow of what you did was good, just some tweaks to make it fit with our
> coding style, etc.


No. That is exactly what i wanted. We are talking about consistency here.
That
is vital to every software project that has to be maintained for the next
two or three
decades (and that is the plan, isn't it ;-))

Thank you for taking the time.

Ciao,
  Steffen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20061106/36c3be82/attachment.htm 


More information about the bazaar mailing list