Adds LazyRawTextReader support for reading lists
#617
Conversation
| // Get as much of the sequence's body as is available in the input buffer. | ||
| // Reading a child value may fail as `Incomplete` | ||
| let buffer_slice = self.value.available_body(); | ||
| RawSequenceIterator::new(buffer_slice) | ||
| RawBinarySequenceIterator::new(buffer_slice) |
There was a problem hiding this comment.
🗺️ Now that there are both binary and text readers, RawSequenceIterator has become the RawBinarySequenceIterator.
| @@ -33,34 +34,6 @@ impl<'data> LazyDecoder<'data> for BinaryEncoding { | |||
| // === Placeholders === | |||
| // The types below will need to be properly defined in order for the lazy text reader to be complete. | |||
| // The exist to satisfy various trait definitions. | |||
| #[derive(Debug, Clone)] | |||
| pub struct ToDoTextSequence; | |||
There was a problem hiding this comment.
🗺️ This PR defines the LazyRawTextSequence type and removes the stubbed-out ToDoTextSequence type.
src/lazy/text/buffer.rs
Outdated
There was a problem hiding this comment.
🗺️ This file contains the new parsing logic needed to detect the beginning of a list, a value in the list, and the end of the list.
There was a problem hiding this comment.
🗺️ All changes in this file stem from the change on line 175; see the TODO comment.
| } | ||
| } | ||
|
|
||
| impl<'a> Debug for LazyRawTextSequence<'a> { |
There was a problem hiding this comment.
🗺️ This was copied from the binary sequence's impl. It tries to read the sequence's contents on a best-effort basis. We may be able to think up something more robust, but as a Debug impl on an experimental API, I'm ok with this for the moment.
There was a problem hiding this comment.
Suggestion—can we just use self.value.input.data (assuming it's valid utf-8) and if it fails, then we fallback to the Debug impl of the TextBufferView? This seems like an approach that could work for all lazy text values. What do you think?
There was a problem hiding this comment.
Good idea. Opened #641 to track this.
| Ok((_remaining, None)) => None, | ||
| Err(e) => e | ||
| .with_context("reading the next list value", self.input) | ||
| .transpose(), |
There was a problem hiding this comment.
🗺️ This call to transpose() is the ToIteratorOutput trait's extension method.
src/lazy/text/matched.rs
Outdated
| // TODO: Decide what (if anything) to store here. | ||
| // Storing any collection of bytes or ranges means that this type cannot implement Copy, | ||
| // which in turn means MatchedValue and EncodedTextValue also cannot implement Copy. | ||
| // We probably also don't want to heap allocate just to match the long string. |
There was a problem hiding this comment.
The way I see it, a long string is basically a list of strings that gets treated as a single string. I would lean toward storing nothing here, just like MatchedValue::List. While we probably do need a heap allocation to read the string, I agree that we should not be performing heap allocations to match the string (or any value, really) since that is unnecessarily eager.
| } | ||
| } | ||
|
|
||
| impl<'a> Debug for LazyRawTextSequence<'a> { |
There was a problem hiding this comment.
Suggestion—can we just use self.value.input.data (assuming it's valid utf-8) and if it fails, then we fallback to the Debug impl of the TextBufferView? This seems like an approach that could work for all lazy text values. What do you think?
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #617 +/- ##
==========================================
- Coverage 81.72% 81.64% -0.09%
==========================================
Files 119 120 +1
Lines 21778 21876 +98
Branches 21778 21876 +98
==========================================
+ Hits 17799 17860 +61
- Misses 2331 2366 +35
- Partials 1648 1650 +2
☔ View full report in Codecov by Sentry. |
Builds on outstanding PRs #609, #612, #613, #614 and #616.
Adds support for reading lists to the
LazyRawTextReader.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.