Code review comment for lp:~vila/bzr/389648-hook-calls-base

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Review: Approve
    jam> As mentioned above, the target was wrong, it should be 'lp:bzr'
    jam> which is ~bzr-pqm/bzr/bzr.dev, not ~bzr/bzr/trunk.

Amazing, obviously I clicked the wrong button... no idea why nor
when... that wasn't intended anyway.

    jam> Anyway, I did the review locally. It was still a bit hard to
    jam> follow, because a lot of 'formatting' updates. Stuff like:

    jam> -class TestHook(TestCase):
    jam> +class TestHook(tests.TestCase):

Yeah :-(

In the past I've sent these trivial updates directly without
review, but then you commented that may be I should at least send
them so that people know about them even without waiting for an
approval.

I tried a middle term approach here, because I thought there was
only a few of them (but you call them 'a lot' :-)...

I don't mind fixing them as I encounter them, I don't want to
make the reviews harder than they have to either, but I'd like a
minimal effort workflow to get these fixes landed too.

    jam> I wonder if this wouldn't be like the whitespace fixes, and we
    jam> should have "one big patch" across the codebase to fix them,
    jam> rather than getting them piecemeal. Then again, the same
    jam> arguments apply for and against that were present for the
    jam> whitespace updates...

Yeah, exactly the same arguments. But again, I feel that I spend
less energy fixing them that discussing how to fix them all,
so...

    jam> Anyway, I think the basic fix here was:

You're right.

<snip/>

    jam> Which seems just fine.

Ok.

    jam> I should comment that using "super()" is not always
    jam> correct anymore.

    jam> To start with, it only matters when you have multiple
    jam> inheritance. But even more importantly, python 2.6
    jam> 'broke' super(...).__init__() if there are any
    jam> arguments.

I disagree. python-2.6 is just stricter and catch bogus calls.

    jam> Because object.__init__() no longer allows
    jam> arguments. (So as near as I can tell, there is no way to
    jam> safely __init__ a multiple inheritance structure that
    jam> wants arguments passed as part of init.

Yes there is, you handle arguments where they are expected and
you don't pass them where they are not.

The cases I had to fix when I addressed python-2.6 compatibility
weren't that complex once I understood what was going on.

And as I remember the only case that seemed controversial at the
time was indeed an abuse.

<snip/>

    jam> Anyway, IMO super().__init__() was broken in 2.6, though
    jam> certainly someone thought it was the right thing to do.

Well, it would be good to agree on that and decide whether we
want to use super() or not.

I refrained to fix all calls to Hooks.__init__ because I
considered it was slightly out of scope for this patch but only
slightly.

If you can provide examples in the actual code base where calling
super().__init__ will be bogus, I'll be happy to revise my
opinion.

        Vincent