Code review comment for lp:~jameinel/bzr/1.17-rework-annotate

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

...
>
> A couple of comments on the class API:
>
> - _update_needed_children() and _get_needed_keys() sounds like
> good candidates for Graph() or some specialization of it.

True, though they also have side effects like removing texts when there
are no more needed children, etc.

>
> - _update_from_one_parent() the doc string says first parent, why
> not call it _update_from_first_parent() then ? Unless you envision
> some other possible usage...

Sure.

>
> - add_special_text(), hmm, what's that ? The doc string doesn't
> help a lot :-) Does that need to be public ?

It does, as it is used by WorkingTree to add the 'current:' text to be
annotated. (One other benefit of this new code is that 'bzr annotate
NEWS' after a merge doesn't annotate both parents independently... \o/)

>
> jam> 4) Implement a pyrex version of Annotator, to handle some
> jam> inner-loop functionality. (Nicely enough, you can still subclass
> jam> from it.)
>
> It would be nice to defines in pyrex *only* those inner-loops,
> not a requirement to land that patch tough.
>

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpThhgACgkQJdeBCYSNAAN5GACgvRr9AoVrnRJ/u4Gd2nPEjAil
mTcAn3q07M6kiM6qaerO6ldZRQSOp0wh
=9LiN
-----END PGP SIGNATURE-----