Weave.join(), and progress indicator fixes

[Robert Collins]

On Sun, 2006-01-29 at 18:57 -0600, John Arbash Meinel wrote:
> 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.

Not true. If you *assign* to a class variable via self, then it becomes
an instance variable, but plain modification has no impact.

class Foo:
  _bar = {}

  def something(self):
    self._bar['gam'] = 'quux'
    assert self._bar is Foo._bar, "this will pass"


Its this inconsistent behaviour that makes class scope variables -really
bad- initializers for instance variables. Yes its legal python, but IMO
its bad python.


>  I don't think we have to add an __init__, I thought
> they weren't good for test classes.

__init__ in *test cases* must support the contract klass(methodname) ->
a test instance that will run methodname. setUp is used in TestCase
subclasses so that resources needed for the test are created just in
time, rather than at the test case instantiation.


> I suppose I could do:
> 
> def setUp(self):
>   TestCase.setUp(self)
>   self._extract_count = 0
> 
> I can do it if you request.

Eh? This is in the instrumented weave, its not a test case and doesn't
need anything special:

    def __init__(self, weave_name=None):
        self._extract_count = 0
        super(InstrumentedWeave, self).__init__(weave_name)



> > 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).

Please dont add setUp, that would be ugly :).... And yes, your current
pattern works but its harder to read, particular for people not familiar
with the intimate details of bindings vs variables.

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: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060130/ba3b2beb/attachment.pgp