[MERGE] lazy inventory writes

[Robert Collins]

On Thu, 2006-10-12 at 12:18 -0600, Richard Wilbur wrote:
..
> This looks like a nice start in a good direction.



> ...> 
> 
> The comments in this section contain a good number of well-punctuated
> but un-capitalized sentences?  Some other comments amount to phrases
> punctuated as sentences, and still others, un-punctuated sentences.
> 
> It is a nice working narrative, but it would read more easily with some
> help.


I find Capitals in such narrative distract me, probably for no good
reason - but it is actually deliberate. I'm going to merge this branch
now for 0.12, as it has +2, but we can come back to this - there are
many many more instances of it if we decide it matters.


> > === modified file 'bzrlib/workingtree.py'
> > --- bzrlib/workingtree.py       2006-09-27 20:27:46 +0000
> > +++ bzrlib/workingtree.py       2006-10-05 07:42:06 +0000
> 
...
> _set_inventory() parameter 'dirty' is used as a keyword parameter in
> several invocations but not declared as one in method definition.  (What
> should the default value be?)

As I understand python, _set_inventory (with the prototype 
    def _set_inventory(self, inv, dirty):
) has three mandatory parameters, of which 1 must be positional and two
may be keyword or positional. I think you were meaning 'should dirty be
optional' (it should not), or possibly 'why are you using keyword
notation to provide this mandatory parameter?' (we are doing that for
clarity - passing True or False in positional arguments is quite unclear
for people reading the code unless they read the function
definition/prototype too).

> [...]
> > @@ -843,6 +865,17 @@
> >          else:
> >              return '?'
> >  
> > +    def flush(self):
> > +        """Write the in memory inventory to disk."""
> 
> How about """Write the inventory from memory to disk."""?
> Alternatively, """Commit the inventory to disk."""?

I think that the current wording is more clear, though perhaps
hyphenating the composite to in-memory would make it more clear. Commit
would be a dangerous word to use as it implies atomicity and a
relationship to 'committing' which is not present in the code base.


> > +        :raises errors.InventoryModified: When the current in memory
> > +            inventory has been modified, read_working_inventory will
> > +            fail.
> The phrase "read_working_inventory will fail" seems a good deal less
> specific than the goal of the declaration.
> If you take the verb "raises" to begin the thought, it could read more
> easily as
>  :raises errors.InventoryModified: when the current inventory has
> uncommitted modifications.

'current inventory' is too ambiguous here..

> Another possibility,
>  :raises errors.InventoryModified: when the inventory currently cached
> in memory has uncommitted modifications.

This is better, but if we want a more pithy sentence, how about we trim
from the ,:
        :raises errors.InventoryModified: When the current in memory
            inventory has been modified.

I note that you were not capitalising the beginning of the sentence, my
current preference is to make the explanation of parameters, exceptions
and other epydoc markers into proper sentences, not continuations from
the markup. I do this because there is no guarantee that the two will be
formatted side by side like this - they could easily be in tabular form
where there is not such an easy flow from one to the other.

I'm reversing the sentence to read: 
 'read_working_inventory will fail when the current in-memory inventory
has been modified.'
Note that the lower case first letter is because it is the name of the
method.




> > +            # unlock in this order so that the unlock-triggers-flush
> > in
> > +            # WorkingTree is given a chance to fire.
> Without the first word of the sentence capitalized, it is natural to
> look above in the code in an attempt to find the beginning of the
> sentence.  If this is supposed to refer to the method named unlock(),
> instead of the common English verb, the parentheses would help.

I agree here - I've changed this to begin with a capital.

> I like where this is going.  Look forward to your comments.

Thanks for the detailed review. If I may give you some feedback on your
review itself. What you have focused on appears to be just the shiny
mechanics of creating polished code, not the machinery or design that is
actually being put forward. If you have considered that as well, saying
so (for instance, 'I have some spelling and grammar corrections but the
design and implementation look sound') explicitly may be of value.

In your review you appear to have kept most of the content, which makes
the review longer to read: please trim the context a little more
aggressively. We need to keep the review understandable, but the person
being reviewed has the branch handy to refer to.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061013/7e0fda74/attachment.pgp