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.
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 git_repository. GitUbuntuReposi tory repo: the repository to use." Repo(.. .)", e.g.
- 'repo' parameter could use a doc, e.g.:
":param gitubuntu.
- Add comment to explain "repo_builder.
"# 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 parent_ commit is not None, but parent_ commits) != 0. Logically this parent_ commits MUST be a list; if None ever got passed parent_ commits is always assumed to be a list,
√ Previously the code checked that changelog_
now it checks len(changelog_
works fine, the one caveat being that this now implies an assumption
that changelog_
in then things will break. I did not spot any places where this is
likely to happen, though.
- Given that changelog_
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 changelog_ parent_ commits( ) would be nice to add
- Param docs for test_get_
√ Otherwise, LGTM
7. Add multiple changelog parent tests
√ LGTM
8. Support multiple changelog parents parent_ commits feels a bit
√ The list comprehension for calculating changelog_
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.