Merge lp://staging/~bigkevmcd/ubuntu-system-image/some-tidyups into lp://staging/ubuntu-system-image/server
Proposed by
Kevin McDermott
| Status: | Rejected |
|---|---|
| Rejected by: | Stéphane Graber |
| Proposed branch: | lp://staging/~bigkevmcd/ubuntu-system-image/some-tidyups |
| Merge into: | lp://staging/ubuntu-system-image/server |
| Diff against target: |
516 lines (+224/-111) 9 files modified
bin/copy-image (+1/-1) bin/generate-keyrings (+1/-1) bin/import-images (+1/-1) lib/systemimage/config.py (+4/-0) tests/generate-keys (+1/-1) tests/run (+1/-1) tests/test_config.py (+206/-99) tests/test_static.py (+8/-6) utils/check-latest (+1/-1) |
| To merge this branch: | bzr merge lp://staging/~bigkevmcd/ubuntu-system-image/some-tidyups |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Stéphane Graber (community) | Disapprove | ||
|
Review via email:
|
|||
Description of the change
This is just a quick tidyup as part of preparing to take on maintenance.
I split out all the config tests, and removed hard-coded Python references.
To post a comment you must log in.
Unmerged revisions
- 167. By Kevin McDermott
-
Some tidyups.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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