Weave.join(), and progress indicator fixes

[John Arbash Meinel]

Robert Collins wrote:
> On Wed, 2006-01-25 at 12:06 -0600, John A Meinel wrote:
>> Can I get a review?
>>
>> John
>> =:->
>> plain text document attachment (fix-weave-join.patch)
> 
>> === modified file 'bzrlib/tests/test_weave.py'
>> --- bzrlib/tests/test_weave.py	
>> +++ bzrlib/tests/test_weave.py	
>> @@ -991,3 +991,64 @@
>>          self.assertRaises(errors.WeaveInvalidChecksum, w.check)
>>  
>>
>> +class InstrumentedWeave(Weave):
>> +    """Keep track of how many times functions are called."""
>> +    
>> +    _extract_count = 0
>> +
>> +    def _extract(self, versions):
>> +        self._extract_count += 1
>> +        return Weave._extract(self, versions)
>> +
> 
> 
> This looks suspect to me - _extract_count is surely per instance not per
> class.

If you modify a class variable through the self pointer, it becomes an
instance variable. I don't think we have to add an __init__, I thought
they weren't good for test classes.

I suppose I could do:

def setUp(self):
  TestCase.setUp(self)
  self._extract_count = 0

I can do it if you request.

> 
> I'd add __init__ here for clarity, even if in this case the attribute is
> being set onto the instances correctly.
> 
> ...
> 
>> +        self.assertEqual(4, w2._extract_count)
>> +
>> +
>> +    def test_double_parent(self):
> 
> PEP8 - one line between methods.
> 

Sorry I missed that one.

> 
> 
> +1 with those addressed.
> 
> Rob

I'll add setUp() and merge it in. I'm curious, if is it really
necessary? I've used this pattern before, and I've never had any
problems. Class._foo is the class local, but self._foo will be replaced
(since assignment is actually binding a new variable, list modification
would modify inline).

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 252 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060129/db1c8bdd/attachment.pgp