Bazaar

Code review comment for lp:~abentley/bzr/nested-trees-composite-tree

nested-trees-composite-tree
Merge into trunk-old

Revision history for this message

Martin Pool (mbp) wrote on 2009-05-12:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Martin,
>
> You wanted some documentation of CompositeTree to be included with the
> patch that introduces it. Since NestedTrees is also introduced by this
> patch and is arguably more important in the long run, I've included
> documentation for it as well.
>
> I hope this is the kind of thing you were looking for.

So yes, it is.

I wonder if this would be better described in the overview of major classes (in HACKING, I think?) than separated out into something only about nested trees. If you were going to add a new documentation file you should link it into the index, but here I think it's better not to add one.

I think for ReST you need a blank line between the heading underline and the following text?

In CompositeTree, I'd like to see something about how you obtain one.

It's good to write it up like this because it makes the issues more clear. For example, what will happen if code gets a CompositeTree and tries to treat it like a regular tree? Will it just error? (Some of these can be discussed in the thread about the general design but probably they should be answered in the docs.) If "by con

1279 +It is constructed from a Tree (typically a RevisionTree or WorkingTree) and a
1280 +Branch.

If you mean you just do

CompositeTree(top_tree, branch)

then a code sample to say so would be unambiguous.

+
+NestedTrees
+-----------
+NestedTrees is an API providing access to a set of nested trees. It is able to
+convert paths and file-ids into references to specific Trees. It provides
+caching, to avoid retrieving the same Tree multiple times. It provides
+locking, to ensure trees are locked and unlocked at appropriate times.
+
+It is used by CompositeTree, and is a recommended API for all other operations.
+It is expected to acquire more capabilities as the needs of sets of nested
+trees become clearer.

Names that sound like they're plurals of some other class name are confusing. (Is this about ``NestedTree``, which sounds like a kind of tree?) Maybe NestedTreeSet or TreeNest?

Again I'd ask how you get one of them.

Thanks

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Hi Martin,
> 
> You wanted some documentation of CompositeTree to be included with the
> patch that introduces it.  Since NestedTrees is also introduced by this
> patch and is arguably more important in the long run, I've included
> documentation for it as well.
> 
> I hope this is the kind of thing you were looking for.

So yes, it is.

I wonder if this would be better described in the overview of major classes (in HACKING, I think?) than separated out into something only about nested trees.  If you were going to add a new documentation file you should link it into the index, but here I think it's better not to add one.

I think for ReST you need a blank line between the heading underline and the following text?

In CompositeTree, I'd like to see something about how you obtain one.

It's good to write it up like this because it makes the issues more clear.  For example, what will happen if code gets a CompositeTree and tries to treat it like a regular tree?  Will it just error?  (Some of these can be discussed in the thread about the general design but probably they should be answered in the docs.)  If "by con

1279	+It is constructed from a Tree (typically a RevisionTree or WorkingTree) and a
1280	+Branch.

If you mean you just do

CompositeTree(top_tree, branch)

then a code sample to say so would be unambiguous.

+
+NestedTrees
+-----------
+NestedTrees is an API providing access to a set of nested trees.  It is able to
+convert paths and file-ids into references to specific Trees.  It provides
+caching, to avoid retrieving the same Tree multiple times.  It provides
+locking, to ensure trees are locked and unlocked at appropriate times.
+
+It is used by CompositeTree, and is a recommended API for all other operations.
+It is expected to acquire more capabilities as the needs of sets of nested
+trees become clearer.

Names that sound like they're plurals of some other class name are confusing.  (Is this about ``NestedTree``, which sounds like a kind of tree?)  Maybe NestedTreeSet or TreeNest?

Again I'd ask how you get one of them.

Thanks

« Back to merge proposal