Code review comment for lp:~ricardokirkner/locolander/celery-tasks

Revision history for this message
Ricardo Kirkner (ricardokirkner) wrote :

> Looks good!
>
> One note: you should not import os.path, but import os. From the
> help(os.path):
>
> "Instead of importing this module directly, import os and refer to this module
> as os.path."

fixed

>
> Any reason to change the prefix on l.94-95?

because the extra '-' is unnecessary; it will be added by make_random_string, so having it in the prefix will result in a double '-' in the final string

>
> The import from l.158 should have a blank line separating import from stdlib
> from imports from third-party.

fixed

>
> When needing the same patch in more than one test, and specially when the same
> patch is needed in every test inside the test case, I advice to do the
> patching in the setUp method, so we can have the patch in a single place and
> don't change the signature of every method (but if you disagree, you can leave
> as is.
>

having them in the methods is a bit nicer sometimes as it keeps context local. have changed as you suggested anyway, as it's applied to all tests.

> Approving since none of the above is a blocker.

« Back to merge proposal