Page MenuHomeSoftware Heritage

Document edge cases of content_get_range.
ClosedPublic

Authored by vlorentz on Nov 21 2018, 6:32 PM.

Diff Detail

Repository
rDSTO Storage manager
Branch
content_get_range-edge-cases
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2515
Build 3113: tox-on-jenkinsJenkins
Build 3112: arc lint + arc unit

Event Timeline

douardda added a subscriber: douardda.
douardda added inline comments.
swh/storage/storage.py
227

s/function/generator/

Also are you sure about your assertion there? In *this* implementation at least, it does not seems correct.

Reading this code (and especially the fact it may generate None values), it looks to me that the intention was that this generator always generates as many values as was given in the content parameter (not sure why nor if it is needed somewhere, instead of of not yielding None values, but that's another fine-tune-API debate).

This revision now requires changes to proceed.Nov 22 2018, 9:57 AM
ardumont added inline comments.
swh/storage/storage.py
265

I'm not sure i follow what you meant here, can you please clarfy?

267

It's not supposed to nor does it, doesn't it?

If it's the case, adding tests case to demo this would be appreciated.
So that we can fix after that ;)

vlorentz marked 5 inline comments as done.
  • fix doc
swh/storage/storage.py
227

Also are you sure about your assertion there? In *this* implementation at least, it does not seems correct.

Oops, indeed. I updated the docstring to mention that for now it returns exactly as many items, but that it may change in the future.

267

Ugh, I don't know why I wrote this. And that's not even a generator.

swh/storage/storage.py
265

I believe it's true only because of the sha1 collision case (identifiers are sha1 ids).
Other than that, that should not be the general case here.

vlorentz added inline comments.
swh/storage/storage.py
265

Yup, that's what I meant. I believe that's worth mentioning.

This revision is now accepted and ready to land.Nov 27 2018, 11:43 AM
This revision was automatically updated to reflect the committed changes.