[MERGE] Revert merge and pull display using TreeTransform._iter_changes

Aaron Bentley aaron.bentley at utoronto.ca
Wed Feb 14 21:43:08 GMT 2007


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel wrote:
> +            count = tree_to.pull(branch_from, overwrite, rev_id,
> +            delta.ChangeReporter(tree_to.inventory))
> 
> ^- can you indent this, so it is clearer you are continuing the previous
> line?

Glad to.

> +        if None not in name and None not in parent_id and\
> +            (name[0] != name[1] or parent_id[0] != parent_id[1]):
>              renamed = True
>          else:
>              renamed = False
> 
> ^- "None not in parent_id" looks incorrect. It might be "None not in
> parent_ids" or "parent_id is not None"

Looks incorrect, but is not, since they are tuples.

> You might consider using plurals for variables that contain more than
> one value. "None not in name" is a lot more confusing than "None not in
> names".

Stylistically, I prefer name[0] over names[0], and these pairs are meant
to be used that way.  The naming of these variables is consistent with
other places iter_changes is used, and with the implementation of
iter_changes, itself.

> v- Incorrect import line. There are quite a few places I've seen you do
> this. It really should be: "from bzrlib import delta".

Sorry.

> And really that
> should be done up at the top of the module, possibly in a lazy_import
> statement.

Okay, I guess I should learn how that's done...

> v- This function seems a little long. Are there reasonable places that
> you could break it up? There may not be. But I generally avoid super
> long functions.
>
> +    def _iter_changes(self):

Well, I've broken it into four pieces, at the cost of two extra function
call in the loop.  But it's not too bad, since it scales with the number
of changes, not the size of the tree.

And if you call that super long, you ain't seen nothin'. :-)

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF04Jr0F+nu1YWqI0RAuk6AJ9VyspmbiC92KvTLRXq+5m29+KeCgCgiFV6
7Jljb/pUw7mlIuAJ3JqUiPg=
=Bi5O
-----END PGP SIGNATURE-----



More information about the bazaar mailing list