utils.grouper: Improve implementation
ClosedPublic
Actions

Authored by ardumont on Dec 7 2018, 7:11 PM.

Details

Reviewers

vlorentz

Group Reviewers

Reviewers

Commits

rDCORE475377069edc: utils.grouper: Avoid potential bug of input data matching stop_value
rDCOREaeac32dba810: utils.grouper: Rename fillvalue to stop_value and fix docstring
rDCOREddecea090301: utils.grouper: Open fillvalue argument to function

Summary

Loader-tar defines a slightly different utils.grouper to deal with iterable of tuples.
This improves the generic definition to deal with that case.
That allows to remove some code in the loader-tar.

Related T1411

Test Plan

tox

Diff Detail

Repository

rDCORE Foundations and core functionalities

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 2938
Build 3738: tox-on-jenkins	Jenkins
Build 3737: arc lint + arc unit

Event Timeline

ardumont created this revision.Dec 7 2018, 7:11 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptDec 7 2018, 7:11 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCORE/job/tox/64/ for more details.

Harbormaster completed remote builds in B2938: Diff 2517.Dec 7 2018, 7:13 PM

ardumont added a child revision: D794: loader.tar: Add some tests and remove dead code.Dec 7 2018, 8:04 PM

ardumont added a reviewer: vlorentz.Dec 7 2018, 8:04 PM

vlorentz requested changes to this revision.Dec 8 2018, 9:30 AM

vlorentz added inline comments.

swh/core/tests/test_utils.py
46	That line should be `[(8, 9), (None, None)]`
56	That line should be: self.assertEqual(out, [[9, 8, 7, 6], [5, 4, 3, 2], [1, 'a', 'a', 'a']])
swh/core/utils.py
44–45	Replace the for loop with what the grouper from the tarloader does: return itertools.zip_longest(*args, fillvalue=fillvalue) You don't want to drop the fill values (particularly if one of the items in the iterator happens to be equal to the fill value).

This revision now requires changes to proceed.Dec 8 2018, 9:30 AM

ardumont marked 3 inline comments as done.Dec 8 2018, 10:54 AM

ardumont added inline comments.

swh/core/tests/test_utils.py
46	According to the wrong docstring (because incomplete), you are correct!
56	According to the wrong docstring (because incomplete) you are correct!
swh/core/utils.py
44–45	I hear you but yes we do. I realize the docstring is wrong again and the prototype must be changed as well. The actual callers (quite some with the None as fillvalue) expects that. Also, fillvalue is both a fillvalue and a stop value that we don't want involved in the last block. Well, that's how i expected the function to be used initially (and is used as far as i can tell). What i need to do: the first line of the docstring should be improved to mention that (`Collect data into fixed-length blocks except for the last block.That last block will contain only the remaining number of elements.`). fillvalue should be renamed to stopvalue in the grouper prototype (even though it will be used as fillvalue in the implementation).

vlorentz added inline comments.Dec 8 2018, 11:00 AM

swh/core/utils.py
44–45	Then you can drop the `fillvalue`/`stopvalue` argument and always set it with `stopvalue=object()`. This is guaranteed to be unique.

ardumont marked an inline comment as done.Dec 8 2018, 11:10 AM

ardumont added inline comments.

swh/core/utils.py
44–45	Then you can drop the fillvalue/stopvalue argument and always set it with stopvalue=object(). This is guaranteed to be unique. Nice, that would address `(particularly if one of the items in the iterator happens to be equal to the fill value).` better. Better than assuming it won't happen as nothing really enforces this). Thanks!

utils.grouper: Rename fillvalue to stop_value and fix docstring
utils.grouper: Avoid potential bug of input data matching stop_value