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

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

> There is little to comment on given the detailed cover letter, I
> like the cleanup (to come ? :) in annotate and the introduction
> of the Annotator class, but since you intend to build policy
> classes as front-end, why not make the class private until you
> feel more confident about the overall API ? Like you, I'm not
> sure the GUIs will really need to access that class...

I'm not sure if you understood me correctly. Annotator is *meant* to be
the final api for getting an annotation for various versions of a file.

An AnnotationPolicy is meant to be the way to say "I want to ignore
whitespace", etc.

I can make it hidden, but since:

VF.get_annotator() is meant to be the public api that
qannotate/gannotate will use...

...
>
> Pretty much:
>
> - extract the date from the revids of the annotations (only the
> first two ones),
> - return the oldest
>
> It would be very appreciated to not break the actual result or at
> the very least provides a way to get the same functionality
> before 1.17 is out.

So my idea was to do:

_break_annotation_tie = None

if _break_annotation_tie is not None:
  # mutate the data to fit the old api
else:
  # do it my way

...

> jam> + the_heads = heads(annotation)
> jam> + if len(the_heads) == 1:
> jam> + for head in the_heads:
> jam> + break
>
> That's a really funny way to write head = heads[0]... Do I miss
> something ?

heads is a set, you can't do set()[0], you have to:

list(heads)[0]

iter(heads).next()

heads.pop() # though I believe it is a frozenset and this is illegal

[head for head in heads][0]

for head in heads:
  continue

for head in heads:
  pass

for head in heads:
  break

Take your pick. The last one is the fastest because it evaluates the
iter() inline, and doesn't have a function call. Nor does it build an
intermediate list. And for whatever reason, TIMEIT says that 'break' is
faster than the others.

>
> <snip/>
>
> jam> === added file 'bzrlib/_annotator_pyx.pyx'
> ...
> jam> +class Annotator:
> ...
> jam> + def _update_needed_children(self, key, parent_keys):
> jam> + for parent_key in parent_keys:
> jam> + if parent_key in self._num_needed_children:
> jam> + self._num_needed_children[parent_key] += 1
>
> +=1 ? Hmm, I like that pyrex version you're using, send me some :)

Actually for pyrex 0.9.8 you can even do:

cdef list foo

and then *it* will translate

foo.append(...)

into

PyList_Append(foo, ...)

It would be *really* nice to depend on 0.9.8.5 as it would clean up
certain bits. (Note it only really supports lists, it allows:
  cdef dict foo
  cdef tuple foo

and will do runtime checking, etc, but it doesn't have any smarts about
set_item /get item/append, etc.
)

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

iEYEARECAAYFAkpTftEACgkQJdeBCYSNAAM/TgCfdlmsdNtMT2+t+lFRsL3QTgVr
E8AAmwdgejnYx0JK1rWMOIrEBeQJJfSN
=jaUI
-----END PGP SIGNATURE-----