Fix per-setter progress counter being cumulative in ContentImporter#93
Conversation
The metadata-setter loop used `enumerate(data, start=index)`, carrying the
counter across setters. As a result `{setter}: Handled N items...` reported
a cumulative count over all prior setters (and the content-construction
loop) instead of the items handled by that setter, producing misleading
log output on larger imports.
Restart enumeration per setter and add a caplog regression test that
exercises the per-setter counter via `IMPORTER_REPORT=1`.
|
Érico Andrei the email address in your commit does not match an email in your GitHub account. Thus it is impossible to determine whether you have signed the Plone Contributor Agreement, which is required to merge this pull request. Learn about the Plone Contributor Agreement: https://plone.org/foundation/contributors-agreement If you have sent in your Plone Contributor Agreement, and received and accepted an invitation to join the Plone GitHub organization, then you might need to either add the email address on your Agreement to your GitHub account or change the email address in your commits. If you need to do the latter, then you should squash the commits with your matching email and push them. Add more emails to your GitHub account: Change the email address in your commits: |
|
@ericof thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment: To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
|
Tests are failing. Looks like plone.api still imports pkg_resources, but you must have a newer version of setuptools which doesn't include it |
|
Érico Andrei the email address in your commit does not match an email in your GitHub account. Thus it is impossible to determine whether you have signed the Plone Contributor Agreement, which is required to merge this pull request. Learn about the Plone Contributor Agreement: https://plone.org/foundation/contributors-agreement If you have sent in your Plone Contributor Agreement, and received and accepted an invitation to join the Plone GitHub organization, then you might need to either add the email address on your Agreement to your GitHub account or change the email address in your commits. If you need to do the latter, then you should squash the commits with your matching email and push them. Add more emails to your GitHub account: Change the email address in your commits: |
|
Érico Andrei the email address in your commit does not match an email in your GitHub account. Thus it is impossible to determine whether you have signed the Plone Contributor Agreement, which is required to merge this pull request. Learn about the Plone Contributor Agreement: https://plone.org/foundation/contributors-agreement If you have sent in your Plone Contributor Agreement, and received and accepted an invitation to join the Plone GitHub organization, then you might need to either add the email address on your Agreement to your GitHub account or change the email address in your commits. If you need to do the latter, then you should squash the commits with your matching email and push them. Add more emails to your GitHub account: Change the email address in your commits: |
mauritsvanrees
left a comment
mauritsvanrees
left a comment
There was a problem hiding this comment.
The Agreement check fails. That seems because the first commit is done with your plone.org account. It may help if you add that to the list of known email addresses in your GitHub account.
But I approve of course, you are sitting right beside me. :-)
4 participants
Summary
Fixes #92.
The metadata-setter loop in
ContentImporter.do_importusedenumerate(data, start=index), carrying the counter across setters. As a result the per-phase log line{setter}: Handled N items...reported a cumulative count over all prior setters (and the content-construction loop above it) instead of items handled by that setter — producing misleading numbers that on larger imports went well above the total number of objects.The fix restarts enumeration per setter (
start=1).Changes
start=index→start=1on the inner setter loop.TestImporterProgressLoggingclass. MonkeypatchesIMPORTER_REPORT=1so every iteration logs, then asserts each setter's first reported count is1(i.e. the counter resets).Test plan
make test— 248 existing tests still pass.pytest tests/importers/test_importers_content.py::TestImporterProgressLogging— 2 new tests pass.make format/make lintclean.