Clarify docs for `Offset` and `Stream` #826

onkoe · 2025-09-27T17:25:31Z

Goal

Improve the Offset + Stream docs.

Background

I implemented Stream for a type yesterday, but it took quite a lot of time to understand what was going on. The existing docs weren't very clear, so it kinda required me to copy what &[u8] and &str were doing.

After implementing it successfully, I thought I may as well share what I learned! (and spare myself the time re-learning it in the future)

These changes should clarify what an implementation should do... without being too over the top. If I still sounded a little off, or anything is incorrect, please let me know and I'll fix it up! :D

Changes

Improve docs on winnow::stream::Stream...
...and on winnow::stream::Offset.
Fix a couple broken intradoc links.

Testing

I know it'll run in CI, but I tested my changes like so:

typos
committed
cargo doc

previously, the docs in `Offset` and `Stream` made implementing it for a type yourself pretty difficult. I had some trouble, so thought I'd spare myself the time in the future by improving it for everyone :D

epage · 2025-09-29T14:54:31Z

src/stream/mod.rs

+///
+/// Types can implement this trait to become custom input types.


This feels redundant

Suggested change

///

/// Types can implement this trait to become custom input types.

epage · 2025-09-29T14:55:51Z

src/stream/mod.rs

+    /// An iterator containing a tuple of:
+    ///
+    /// - [`usize`]: the offset from the current location.
+    /// - [`Self::Token`]: the token at the `usize` location.


Let's use https://doc.rust-lang.org/std/string/struct.String.html#method.char_indices for inspiration for wording

I guess https://doc.rust-lang.org/std/str/struct.CharIndices.html would be more closely aligned with intent

epage · 2025-09-29T14:56:20Z

src/stream/mod.rs

+    ///
+    /// Note that, since [`Iterator`] is a trait, this type can be anything
+    /// that implements that trait. Therefore, an efficient implementation will
+    /// dynamically create this tuple instead of precomputing it.


This feels oddly specific and more about teaching Rust than explaining this functionality

epage · 2025-09-29T14:57:40Z

src/stream/mod.rs

+    /// Note that this type must implement [`Offset`], which says how many
+    /// bytes have been computed between a checkpoint and some initial/starting
+    /// checkpoint.


This is redundant with the trait bounds and their own documentation as well as inaccurate as we talk about Offsets, not Bytes.

coveralls · 2025-09-29T14:58:40Z

Pull Request Test Coverage Report for Build 18062781148

Details

0 of 0 changed or added relevant lines in 0 files are covered.
7 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-0.1%) to 46.802%

Files with Coverage Reduction	New Missed Lines	%
src/ascii/mod.rs	1	65.96%
src/combinator/multi.rs	2	53.94%
src/stream/mod.rs	2	45.37%
src/stream/range.rs	2	18.18%

Totals
Change from base Build 17156771731:	-0.1%
Covered Lines:	1610
Relevant Lines:	3440

💛 - Coveralls

epage · 2025-09-29T15:00:34Z

src/stream/mod.rs

+    /// A saved location within the [`Stream`].
+    ///
+    /// This type should allow users to reset the stream to a previous state.


Should we have reset link to Self::reset? If so, then we should probably also link to Self::checkpoint()? This also quickly becomes redundant with those.

Maybe

Suggested change

/// A saved location within the [`Stream`].

///

/// This type should allow users to reset the stream to a previous state.

/// A saved location within the [`Stream`].

///

/// See also [`Self::checkpoint`] and [`Self::reset`]

epage · 2025-09-29T15:02:12Z

src/stream/mod.rs

+    /// Creates a [`Self::IterOffsets`] from the existing stream state.
+    ///
+    /// For efficiency, this should probably be computed on the fly, at least
+    /// when it's possible.


This is too implementation focused. Most people will be using an existing stream type. Their goal isn't to create a IterOffsets but to iterate with offsets. Might be good to take inspiration from https://doc.rust-lang.org/std/string/struct.String.html#method.char_indices for wording

epage · 2025-09-29T15:02:37Z

src/stream/mod.rs

    fn iter_offsets(&self) -> Self::IterOffsets;

-    /// Returns the offset to the end of the input
+    /// Returns the number of bytes until the stream completes.


offset, not bytes

epage · 2025-09-29T15:02:57Z

src/stream/mod.rs

    fn eof_offset(&self) -> usize;

-    /// Split off the next token from the input
+    /// Takes the next token from the stream.


This doesn't express that we are also advancing the stream

epage · 2025-09-29T15:03:09Z

src/stream/mod.rs

+    /// This method should consume the token, mutating the original input to
+    /// no longer have it.


We should word this in terms that work for both user and implementer

epage · 2025-09-29T15:04:14Z

src/stream/mod.rs

+    /// Finds the offset of the next value matching the given `predicate`
+    /// function.


Suggested change

/// Finds the offset of the next value matching the given `predicate`

/// function.

/// Finds the offset of the next token that matches `predicate`

epage · 2025-09-29T15:04:52Z

src/stream/mod.rs

+    ///
+    /// The offset is relative from the current location.


I wonder if there is a better way to teach this concept because everything is relative to the stream's current location

epage · 2025-09-29T15:05:15Z

src/stream/mod.rs

+    /// The offset is relative to the current position. This means that an
+    /// input of "0 tokens" will return an offset of `0`.


ditto https://github.com/winnow-rs/winnow/pull/826/files#r2388320264

epage · 2025-09-29T15:05:39Z

src/stream/mod.rs

        P: Fn(Self::Token) -> bool;
-    /// Get the offset for the number of `tokens` into the stream
+
+    /// Find the offset after consuming a number of `tokens` from the stream.


"consuming" makes this sound like this advanced the stream's position.

epage · 2025-09-29T15:07:10Z

src/stream/mod.rs

+    /// **Note:** For inputs with variable width tokens, like `&str`'s `char`,
+    /// `offset` might not correspond with the number of tokens. To get a valid
+    /// offset, use:
+    ///


Did nothing change here? If so, please revert

epage · 2025-09-29T15:07:56Z

src/stream/mod.rs

+    /// Takes a slice of `offset` tokens from the input.
+    ///
+    /// This method should consume `offset` tokens.


I feel like saying "split off" covers both sentences and ensures its all in the summary

epage · 2025-09-29T15:09:10Z

src/stream/mod.rs

    /// # Panic
    ///
-    /// This will panic if
+    /// This method panics if any of the following invariants are violated:


Please revert. This doesn't add anything but does make it more likely for the user to miss whats being said

epage · 2025-09-29T15:10:04Z

src/stream/mod.rs

    fn next_slice(&mut self, offset: usize) -> Self::Slice;
-    /// Split off a slice of tokens from the input
+
+    /// Takes a slice of `offset` tokens from the input.


FYI this is the point that I'm taking a break. Please consider my feedback above on the documentation below and we can re-evaluate once its all been updated

onkoe · 2025-09-29T15:29:58Z

Thanks for your feedback! Note that my primary reason for making these changes is to increase accessibility for implementers. :D

The stable docs on this trait make it overly confusing to implement for the first time. So, I focused on making it easier to understand. If you'd like me to remove those elements and assume that all readers are very experienced winnow users, please let me know!

(will take a better look at these suggestions later this week; your notes on sounding more like the stl and being more careful w/ "consuming" vs. "peeking" are veeeery important)

epage · 2025-09-29T15:35:19Z

The stable docs on this trait make it overly confusing to implement for the first time. So, I focused on making it easier to understand. If you'd like me to remove those elements and assume that all readers are very experienced winnow users, please let me know!

I am fine with making the docs more approachable. My concerns are:

over-optimizing for implementers when the vast majority of people won't implement it
going beyond covering winnow topics to covering general Rust topics. If there is a caveat to why there is special handling needed, you'll need to provide context

onkoe · 2025-09-29T15:39:11Z

Got it! I'll approach this again later with that mindset. Thanks for the feedback 😄

docs(stream): Clarity in Offset and Stream

d60e1a6

previously, the docs in `Offset` and `Stream` made implementing it for a type yourself pretty difficult. I had some trouble, so thought I'd spare myself the time in the future by improving it for everyone :D

epage reviewed Sep 29, 2025

View reviewed changes

		///
		/// Types can implement this trait to become custom input types.

		/// This method should consume the token, mutating the original input to
		/// no longer have it.

		/// Finds the offset of the next value matching the given `predicate`
		/// function.

	/// Finds the offset of the next value matching the given `predicate`
	/// function.
	/// Finds the offset of the next token that matches `predicate`

		/// The offset is relative to the current position. This means that an
		/// input of "0 tokens" will return an offset of `0`.

Clarify docs for Offset and Stream #826

Are you sure you want to change the base?

Clarify docs for Offset and Stream #826

Uh oh!

Conversation

onkoe commented Sep 27, 2025

Goal

Background

Changes

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 29, 2025

Pull Request Test Coverage Report for Build 18062781148

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onkoe commented Sep 29, 2025

Uh oh!

epage commented Sep 29, 2025

Uh oh!

onkoe commented Sep 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Clarify docs for `Offset` and `Stream` #826

Clarify docs for `Offset` and `Stream` #826