Code review comment for lp:~salgado/launchpad/vostok-upload-policy

On Wed, 2010-08-25 at 05:27 +0000, Jeroen T. Vermeulen wrote:
> Review: Approve
> Beautiful. A few notes, of course:
>
> Lines 199 and 319 of the diff have multiple imports that don't conform
> to the new import formatting rules:
>
> from foobar import (
> Bar,
> Foo,
> )

Oops, fixed.

>
> In line 379 of the diff, the "else" and the assertion throw the
> symmetry of the code slightly out of whack. Just an idea, but maybe
> you can move it out into a separate "else" clause?
>
> elif upload.binaryful:
> # [...]
> else:
> raise AssertionError("...")

That wfm.

>
> The list of hand-wrapped text lines at line 382 of the diff puzzles
> me. Why not a single dedent("""triple-quoted multi-line string""")?

That's code I just moved around, but I've changed it as you suggest.

>
> Finally, I think I see either an indentation problem or something

Broken indentation indeed; didn't notice when moving the code around.

> involving tabs around line 390. Did you run "make lint" over the
> current form of this branch?

I never did because 'bzr send' would do that for me, but now that I'm
using bzr lp-submit...

>
> But that's all small fry. The branch is definitely an improvement.

Thanks for the review!

If I can make a suggestion as well, it'd be much better if you could
include chunks of the diff in your review[1], for context, instead of
using the line numbers of the diff. That way we don't have to go back
and forth between your comments and the diff (in my case between my mail
client and the merge-proposal's page), but more importantly because the
diff is a moving target, so there's a big chance that the line numbers
have changed when I get to reply to your review. :)

[1] I do that by replying to the m-p email on my mail client, so the
diff is quoted and I just insert my comments at the appropriate places.