Code review comment for ~racb/git-ubuntu:changelog-parents

Revision history for this message
Bryce Harrington (bryce) wrote :

I have a few very minor suggestions, but looks ok overall.

While this is a big patch, most of the code changes were straightforward to review; there were a few pytest mechanisms I wasn't familiar with, and there were a couple chunks of code that took me some time to untangle, but in the end I didn't spot any logical errors.

Since it's nicely broken up into conceptually distinct patches, I reviewed the code sequentially by patch, so am giving feedback thusly organized, hopefully it'll be clear what lines I'm referring to.

1. importer: drop unused variable
    √ LGTM. Simple cleanup

2. Add test: test_get_existing_import_tags_ordering
    - 'repo' parameter could use a doc, e.g.:
      ":param gitubuntu.git_repository.GitUbuntuRepository repo: the repository to use."
    - Add comment to explain "repo_builder.Repo(...)", e.g.
      "# Construct a synthetic git repository containing tags"
    - Rename 'mock' variable to 'mock_repo'

3. Sort reimport tags before using them
    √ LGTM. Interesting illustration of splitting the test from the fix via xfail.

4. Allow for multiple changelog parents
    √ Previously the code checked that changelog_parent_commit is not None, but
      now it checks len(changelog_parent_commits) != 0. Logically this
      works fine, the one caveat being that this now implies an assumption
      that changelog_parent_commits MUST be a list; if None ever got passed
      in then things will break. I did not spot any places where this is
      likely to happen, though.
    - Given that changelog_parent_commits is always assumed to be a list,
      there is a check around line 607 in _commit_import() that is probably
      unnecessary. It used to be a None check, and now is effectively a
      len()>0 check, however l.extend([]) will be a no-op so no need for a check.

5. test_get_import_commit_msg: multiple parent case
    √ LGTM. I wondered if this could be done as an xfail test case, but it's fine as is.

6. Generalise test_get_changelog_parent_commits
    - Param docs for test_get_changelog_parent_commits() would be nice to add
    √ Otherwise, LGTM

7. Add multiple changelog parent tests
    √ LGTM

8. Support multiple changelog parents
    √ The list comprehension for calculating changelog_parent_commits feels a bit
      dense pythonically but on further review it's just using idioms already
      common in importer.py.

As mentioned in #6, some of the test cases in importer_test.py are documented but not all; would be nice to ensure each of the 3 or 4 test cases added or changed in this patchset also gain docs while we're at it.

review: Needs Fixing

« Back to merge proposal