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

>>>>> "jam" == John A Meinel <email address hidden> writes:

    >> 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...

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

Oh ! Indeed, I understood you wanted trees to be the primary interface.

<snip/>

    jam> So my idea was to do:

    jam> _break_annotation_tie = None

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

Fine.

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

    jam> list(heads)[0]

    jam> iter(heads).next()

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

    jam> [head for head in heads][0]

    jam> for head in heads:
    jam> continue

    jam> for head in heads:
    jam> pass

    jam> for head in heads:
    jam> break

Then I'd go with:

  for head in heads: break # Get head from the set

on a single line to make it more obvious.

That's the first time I see that idiom, I will not be surprised
next time (hopefully, but others can).

<snip/>

    >> +=1 ? Hmm, I like that pyrex version you're using, send me some :)

    jam> Actually for pyrex 0.9.8 you can even do:

Oh ! Yes, jaunty is still at 0.9.7.2 ...

Reading the NEWS about it, I can only agree here.

What is needed to have the package updated ? Host it in the bzr
PPAs ?

     Vincent