[MERGE] annotation policies

[Ian Clatworthy]

John Arbash Meinel wrote:
> John Arbash Meinel wrote:
> | time. Probably the hardest part for me is to get a decent way of testing
> | all of
> | them. So I'd like some feedback about what I've done to date, before I
> | go too
> | much father.

> | For the testing strategy, the plan is to provide different
> | AnnotationScenarios.

> | I'd like some feedback about this structure. It is flexible enough for
> | what I
> | want to do, and I feel comfortable adding more scenarios easily. But it
> | might be
> | overly complex for someone coming along who wants to understand WTF is
> | going on.
> | I need someone else to look at it to tell me so, though.

I found the test code clear, though I'm yet to get my head around exactly
how all the test adaption stuff hangs together.

> | My biggest concern is the disconnect when someone overrides the
> | annotation in an
> | instance. I didn't want to have to retype the whole text, so I have you
> | supply
> | what lines get overridden. But honestly, I can realize that the actual
> | texts are
> | going to be reasonably sized, so it could be a lot clearer if we just
> | require to
> | override the whole text.

Hmm. My initial reaction was that providing the whole texts would be better.
But having read the test code now, I think highlighting just the overrides
as you've done is indeed the better choice.

> | Anyway, I'm pretty happy with where this is going, and I think I can
> | make it
> | perform well and give the user some flexibility with how they want their
> | annotations. Arguably the policy could encompass even more, and have a
> | way to
> | indicate what parent texts it is going to need. (I'm thinking of a
> | left-only
> | annotation policy which should be super fast, and wouldn't need us to
> | extract
> | all of those right-hand parents, and meets the needs of the python
> group of
> | telling you who allowed this code into mainline, rather that what random
> | user
> | wrote it.)

Me too. I think the policy requested by the python guys makes a lot of
sense and I can imagine using that by default. At times though, I'd like
the extra detail provided by other policies.

> | And I suppose it provides identical behavior at the moment, so it might
> | be nice
> | to merge into bzr.dev so I can do incremental updates from here.)

I think you're very close to having this mergeable into bzr.dev. I think we
should do that soon and extend the UI, change the default, add some help,
etc. in a follow on patch. I'm voting resubmit though because some of the
tweaks we've discussed are large enough to warrant a re-review before
merging.

bb:resubmit

test_annotation_policy.py feedback below ...

> +    :ivar _overrides: A dictionary of {revid:[overrides]} where overrides is a
> +        list of (line number, rev id, text) tuples. The annotation listed here
> +        will override the one _annotated_texts

You need to make it clearer here that line numbering is from 0, not 1.

> +    def get_annotated_text(self, revision_id):
> +        """Return the annotated text for requseted revision."""

Spelling of requested is wrong.

> +    def _get_vars_for_scenario(self, policy, scenario):

I'd prefer policy to be renamed policy_name here.

> +        if 'annotation' in debug.debug_flags:
> +            log = trace.warning
> +        else:
> +            log = trace.mutter

I see that you updated the debug flags (help) to support this value. Is it
only enabled during testing? Seems like it would be useful generally, yes?

Everything else looks ok so far. I'm not quite done on the review but I'm
done for today.

Ian C.