Skip to content
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

Adds lazy reader support for long strings #630

Merged
merged 31 commits into from
Sep 1, 2023
Merged

Adds lazy reader support for long strings #630

merged 31 commits into from
Sep 1, 2023

Conversation

zslayton
Copy link
Contributor

Builds on outstanding PRs #612, #613, #614, #616, #617, #619, #620, #621, #622, #623, #627, #628 and #629.

Adds lazy reader support for long strings.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Patch coverage is 92.15% of modified lines.

Files Changed Coverage
src/lazy/text/buffer.rs 89.53%
src/lazy/text/matched.rs 93.18%
src/lazy/text/raw/reader.rs 100.00%

📢 Thoughts on this report? Let us know!.

Copy link
Contributor Author

@zslayton zslayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ PR tour

@@ -30,20 +30,25 @@ use crate::{IonError, IonResult, IonType, TimestampPrecision};

impl<'a> Debug for TextBufferView<'a> {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
const CHARS_TO_SHOW: usize = 64;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Changes in this method are just introducing a named constant and bumping the number of characters printed from 32 to 64.

fail(self)
/// Matches a long string comprised of any number of `'''`-enclosed segments interleaved
/// with optional comments and whitespace.
pub fn match_long_string(self) -> IonParseResult<'data, MatchedString> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ I recommend going to look at the MatchedString enum before reading this implementation. It explains why we're tracking the number of segments and the presence of escaped characters.

Comment on lines -1604 to +1678
input: format!("{input} "), // add trailing space
input: format!("{input}\n0"), // add whitespace and a trailing value
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ Long string syntax is very greedy. It's impossible to declare a long string "complete" until you've run into something that isn't a long string. Later on I'll add support for declaring that the input stream is done, but for the moment this workaround allows me to use the same test infrastructure on long strings.

@zslayton zslayton marked this pull request as ready for review August 21, 2023 02:15
src/lazy/text/buffer.rs Show resolved Hide resolved
src/lazy/text/matched.rs Show resolved Hide resolved
@zslayton zslayton self-assigned this Aug 29, 2023
Base automatically changed from lazy-blobs to main September 1, 2023 15:34
@zslayton zslayton merged commit 4d34d2a into main Sep 1, 2023
18 checks passed
@zslayton zslayton deleted the lazy-long-strings branch September 1, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants