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

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

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

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

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""")?

Finally, I think I see either an indentation problem or something involving tabs around line 390. Did you run "make lint" over the current form of this branch?

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

review: Approve

« Back to merge proposal