Code review comment for lp:~spiv/bzr/per-file-merge-hook-491711

Aaron Bentley wrote:
[...]
> > Exactly how TreeTransform isn't clearly documented that I could find.
>
> How it what? Works?

Yes, that's the missing word. I'm glad your telepathy is functioning
well :)

> > If someone that knows precisely what the operations on it means and
> > could document that it would be a great help, I think.
>
> I've certainly tried in the past, and I'm happy to enhance the
> documentation. Could you start by explaining what is confusing or
> missing from the TreeTransform docstring?

Simply that I didn't find it when I looked for it, believe or not! I
looked for a class docstring on the class that implemented these methods
and instance variables (TreeTransformBase), and then looked for a module
docstring, and then tried grepping doc/developers/. It didn't occur to
me to try looking a couple of classes (and 1000 lines) further down
transform.py (and a couple of classes further down the inheritance
hierarchy too) to see if one of those had introductory documentation.

This is the sort of situation where I miss dedicated “interface”
documentation, I guess.

Thanks for pointing me to that, I thought I'd seen it before but several
minutes of solid searching failed to find it for me! I expect I'll
remember it for next time.

[...]
> is an abbreviation of
> > “transform id” or “transaction id”
>
> Both, as you can see above.

[Eep! Ambiguity of names doesn't help clarity. I realise it's basically
an arbitrary choice, but I suggest picking one. Actually, it's also
that neither name is particuarly satisfying: one of these IDs doesn't
correspond to a single tree transform (which is done as a single
transaction), it corresponds to an element of that transform. So, I'd
probably find “tentry_id” (short for “transform entry ID”) clear and
mnemonic... but it's a bit late to change now :)
]

> >, are they file_ids, or something else
> > but like file_ids?
>
>
> > What's the difference between unversion_file()
> > and delete_contents()?
>
> One schedules the deletion of the contents of a file. The other
> schedules removes the removal of file id of the file. The docstring for
> delete_contents is:
>
> > """Schedule the contents of a path entry for deletion"""
>
> The docstring for unversion_file is:
> > """Schedule a path entry to become unversioned"""
>
> If this is unclear, could you say what about it is unclear?

First I should say I had an intuition about the meanings of these, and I
think my intuition was basically correct. So it's not so bad.

It's unclear for a few reasons:

 * the docstrings don't really add information not already in the method
   name,
 * my confidence in my intuition about the names was low because the
   terms were so close to synonymous, and
 * the terminology is not memorably clear.

By “memorably clear”, I mean that terms like “delete” and “unversion”
are close to synonymous, so the distinction is hard to notice and
recall.

I don't mean say you necessarily should have found better names, maybe
there are no better names for these concepts... none spring to mind for
me! But (assuming I had found the TreeTransform docstring in the first
place) some explicit description of this terminology in the overall docs
would have been helpful.

> I would comment it:
> # However the merge has been resolved, the old contents should not be
> # retained.

Thanks! I've added a comment using these words.

-Andrew.