-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs(iroh-bytes): Improve range-spec docs #1372
Conversation
This attempts to improve the range-spec docs to be a bit more verbose.
Now ready for full review. Please check correctness (some examples have changed!) and how easy it is to understand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM I think, but let's wait for @rklaehn
for the record I tried to make the examples doc test ones, but gave up trying to compare the objects. Something like use range_collections::range_set::RangeSetRange;
let spec = RangeSpec::from([2, 5, 3, 1]);
let ranges = spec.to_chunk_ranges();
let first_range: RangeSet2<_> = (ChunkNum(2)..ChunkNum(7)).into();
let second_range: RangeSet2<_> = (ChunkNum(10)..ChunkNum(11)).into();
let mut iter = ranges.iter();
assert_eq!(iter.next(), Some(first_range)); // didn't find a way to create two comparable objects
assert_eq!(iter.next(), Some(second_range)); // so gave up on that
assert_eq!(iter.next(), None); I think the current state is a really good improvement tbh, a doc test would make it clearer but it's not necessary |
@@ -97,27 +112,45 @@ impl fmt::Debug for RangeSpec { | |||
} | |||
} | |||
|
|||
/// A compressed sequence of range specs | |||
/// A chunk range specification for a collection of blobs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"sequence of blobs"? You could use this for specifying ranges for any sequence of blobs, and calling it a sequence makes it clear why the thing is called RangeSpec Seq ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
/// | ||
/// default is what to use if the children of this RequestRangeSpec are empty. | ||
/// The first item yielded is the [`RangeSpec`] for the first blob in the collection, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. The first item yielded is the RangeSpec for the collection itself.
This is needed e.g. when you already have the collection and don't want to download it again.
But as I mentioned above, I think it is best to leave out the notion of collections and just describe this as a sequence of RangeSet
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i agree that "sequence of blobs" is probably a better description since when using "collection" you already are assuming special treatment of the first item/blob in the collection, which is not what I intended. I fear "sequence of RangeSets" is a bit too abstract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how about this: we remove collection / children from both the docs and the API, and then add another section where we describe where a RangeSpecSeq is used in the case of collections and children (in a subsequent PR)?
I also would like to make the canonical repr mandatory. Without that you can't have structural equality. But better in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
I think we should probably make a note somewhere why the thing is called |
this would be great to add! |
@rklaehn one question not yet answered is the |
I think it is just a bug. It should return the offset of the single non-zero element, instead it only returns Some only if the offset of the single non-zero element is 0, which makes it redundant. But maybe merge this and we can do a second pass? |
Description
This attempts to improve the range-spec docs to be a bit more verbose.
Notes & open questions
Change checklist