Page MenuHomeSoftware Heritage

Document edge cases of content_get_range.
ClosedPublic

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

Diff Detail

Repository
rDSTO Storage manager
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Wed, Nov 21, 6:32 PM
douardda requested changes to this revision.Thu, Nov 22, 9:57 AM
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.Thu, Nov 22, 9:57 AM
ardumont added inline comments.
swh/storage/storage.py
266

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

268

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 updated this revision to Diff 2179.Thu, Nov 22, 11:23 AM
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.

268

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

ardumont accepted this revision.Thu, Nov 22, 12:46 PM
ardumont added inline comments.Thu, Nov 22, 12:56 PM
swh/storage/storage.py
266

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 marked an inline comment as done.Thu, Nov 22, 1:12 PM
vlorentz added inline comments.
swh/storage/storage.py
266

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

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