Code review comment for lp:~bigkevmcd/ubuntu-system-image/some-tidyups

Revision history for this message
Stéphane Graber (stgraber) wrote :

The use of /usr/bin/python and /usr/bin/python3 is deliberate and "/usr/bin/env pythonX" is considered a bad upstream practice.

I'm also a bit confused by your "as part of preparing to take on maintenance" since I'm the maintainer for that code :)

I see quite a few good things in there though:
 - consistent use of find_on_path
 - extra comments

The rest as far as I can tell (though it's hard to catch everything) seems mostly to be about splitting the tests even more which I'd be opposed to. Granularity is good but it can be sufficiently achieved by using the self.assert* functions. Creating separate test functions for all the various cases will create a ton of code duplication and cause the setup/teardown functions to trigger a lot more often (and those are pretty costly).

I'm going to reject this merge proposal for now. I'd really appreciate it if you could come up with separate merge proposals for each chunk of changes so they can be properly reviewed and discussed.

Thanks

review: Disapprove

« Back to merge proposal