Differential D694

Document edge cases of content_get_range.
ClosedPublic
Actions

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

Tags

None

Subscribers

Details

Reviewers

douardda
ardumont

Group Reviewers

Commits

rDSTOCcd2db9b00af0: Document edge cases of content_get_range.
R65:cd2db9b00af0: Document edge cases of content_get_range.
rDSTOcd2db9b00af0: Document edge cases of content_get_range.

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.Nov 21 2018, 6:32 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptNov 21 2018, 6:32 PM

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

Harbormaster completed remote builds in B2515: Diff 2175.Nov 21 2018, 6:37 PM

douardda requested changes to this revision.Nov 22 2018, 9:57 AM

douardda added a subscriber: douardda.

douardda added inline comments.

swh/storage/storage.py
226	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 a subscriber: ardumont.Nov 22 2018, 10: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 ;)

fix doc

swh/storage/storage.py
226	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.

typo

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

Harbormaster completed remote builds in B2519: Diff 2179.Nov 22 2018, 11:29 AM

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

Harbormaster completed remote builds in B2520: Diff 2180.Nov 22 2018, 11:34 AM

ardumont accepted this revision.Nov 22 2018, 12:46 PM

ardumont added inline comments.Nov 22 2018, 12:56 PM

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 marked an inline comment as done.Nov 22 2018, 1:12 PM

vlorentz added inline comments.

swh/storage/storage.py
265	Yup, that's what I meant. I believe that's worth mentioning.

douardda accepted this revision.Nov 27 2018, 11:43 AM

This revision is now accepted and ready to land.Nov 27 2018, 11:43 AM

Closed by commit rDSTOcd2db9b00af0: Document edge cases of content_get_range. (authored by vlorentz). · Explain WhyNov 27 2018, 11:44 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

swh/

storage/

9 lines

Diff 2255

swh/storage/storage.py

Loading...