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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I'm trying to split up a few discussion points into different emails.
Sorry if things get a bit broken. Thunderbird isn't as nice as 'bzr
shelve' :)

Vincent Ladeuil wrote:
>>>>>> "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' :-)...

Your fix was about 2 lines long, and it was about 10 lines of
'tests.TestCase' changes.

so not many, but *more* of those trivial changes than there were actual
content changes.

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

Style patches are always a bit contentious. Far to easy for people to
bikeshed on. Which is why we say "PEP 8" but even that doesn't cover all
the cases of things you want to do.

Similarly stripping the whitespace from all files is trivially easy to
do. (We now have about 300? files that have trailing whitespace again.)

I'd like to say just land trivial cleanups. But on the flip side, your
'trivial cleanups' may cause merge conflicts in any pending work people
are doing. However, we don't want to hold cleaning up the code hostage
for work that may land in the future either.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkr108YACgkQJdeBCYSNAANHTwCgxsrLQOM7+0sySJtYKRW2NSaI
QooAnRk6sP/DUSgjDQjf2+n8NDgKzU7A
=OUbh
-----END PGP SIGNATURE-----