Page MenuHomeSoftware Heritage

lister: Type correctly the 'indexable' column
ClosedPublic

Authored by ardumont on Jun 23 2019, 9:33 AM.

Details

Summary

instead of converting that column as a string

As a side effect, bitbucket wise, we provided improperly the after query
parameter as a date not url encoded. This resulted in improper api response from
bitbucket's (we received from time to time the same next index as the current
one).

Related T1826
Depends on D1632

Diff Detail

Repository
rDLS Listers
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6430
Build 8929: tox-on-jenkinsJenkins
Build 8928: arc lint + arc unit

Event Timeline

Improve dosctrings and logging instruction

Update docstrings and logging instructions

(fix) previous one (my emacs did not update the hash after rebase... and i did
not see it immediately)

bitbucket: Improve pagination api traversal in corner cases

Amend query parameters shenanigans (keep the old behavior)

It made sense to change that behavior at some point but no longer.

ardumont retitled this revision from bitbucket: Improve pagination api traversal in corner cases to lister: Type correctly the 'indexable' column.Jun 25 2019, 2:59 PM
ardumont edited the summary of this revision. (Show Details)
vlorentz added a subscriber: vlorentz.

indexable_type does not seem to be needed as a class attribute; and I'm not a fan of the name.

What about just a simple convert_type function, like you did for the test BB lister?
And also do it for the actual BB lister (it uses iso8601.parse_date twice)

swh/lister/bitbucket/tests/test_bb_lister.py
32 ↗(On Diff #5489)

(I'm asking in another comment later to remove it, but FYI I don't think you need the staticmethod() call here)

This revision now requires changes to proceed.Jun 26 2019, 10:42 AM
swh/lister/bitbucket/tests/test_bb_lister.py
32 ↗(On Diff #5489)

My bad, the method indeed gets bound to the class otherwise

indexable_type does not seem to be needed as a class attribute; and I'm not a fan of the name.

Yes, that name is inadequate.
It's in reference to the (indexer) listers using the column indexable (which is inadequate as well).
I did not want to push too far the fix though ;)

I'll rename the indexable_type to convert_type as suggested though.

This revision is now accepted and ready to land.Jun 26 2019, 11:17 AM
This revision was automatically updated to reflect the committed changes.