[MERGE] lazy inventory writes

[Richard Wilbur]

On Fri, 2006-10-13 at 17:39 +1000, Robert Collins wrote: 
> On Thu, 2006-10-12 at 12:18 -0600, Richard Wilbur wrote:
> ..
> > This looks like a nice start in a good direction.
[...] 
> > ...> 
> > 
> > > === 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).
> 
My syntax concern stemmed from a misreading of the patch.  I am fully in
favor of what you did--I have done a similar thing in languages that
don't support keyword arguments to avoid a relatively meaningless flag
in a list of arguments:
const bool Signed = true;
const bool DECfp = true;
SomeConstructor("name", other, arguments, Signed, DECfp);

> > [...]
> > > @@ -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.
> 
I was confusing VCS language with processor-cache-speak where I more
often deal with caches.  I would have been better served by referring to
the operation as a 'cache flush' or 'flushing the cache'.

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

Sorry about the confusion over my conclusion.  I tried to give the tenor
in:
> > This looks like a nice start in a good direction. 
[...]
> > I like where this is going.  Look forward to your comments.
Next time I'll try to be more explicit in my conclusions as you suggest.

> 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
> 
I erred on the side of context.  Next time it will be more terse.

Thank you very much for your comments.  I have considerable experience
reviewing code in person and face-to-face at my place of employment, and
I am learning about the mechanics of E-mail reviews.  For my employer,
we generally only review very small changes over E-mail.

I'm sold on your solutions to the documentation issues noted above.  I
appreciate your explanations for the stylistic conventions.  I was
feeling the pull for PEP8-ness.

Sincerely,

Richard
-------------- 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/bb59f243/attachment.pgp