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

Fix whitespace remover #9

Closed

Conversation

rsammelson
Copy link
Contributor

Previously, if there was enough whitespace to fill the buffer it would return a read length of zero, even though there is still content to read. This will cause consumers to assume they have read the entire content.

Previously, if there was enough whitespace to fill the buffer it would
return a read length of zero, even though there is still content to
read.
@coriolinus
Copy link
Owner

This is a good catch and a really good idea! However, for style reasons, please apply this patch:

diff --git a/src/whitespace_remover.rs b/src/whitespace_remover.rs
index 7df7b2b..b895119 100644
--- a/src/whitespace_remover.rs
+++ b/src/whitespace_remover.rs
@@ -24,11 +24,11 @@ where
     fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
         let mut my_buf = vec![0; buf.len()];
 
-        Ok(loop {
+        loop {
             let n = self.inner.read(&mut my_buf)?;
             if n == 0 {
                 // the underlying reader is done
-                break 0;
+                return Ok(0);
             }
 
             my_buf.truncate(n);
@@ -37,10 +37,10 @@ where
             // keep reading until we get at least a byte to return
             if !my_buf.is_empty() {
                 buf[..my_buf.len()].copy_from_slice(&my_buf);
-                break my_buf.len();
+                return Ok(my_buf.len());
             }
 
             my_buf.resize(buf.len(), 0);
-        })
+        }
     }
 }

@coriolinus coriolinus mentioned this pull request Aug 12, 2024
@coriolinus
Copy link
Owner

Superseded by #11.

@coriolinus coriolinus closed this Aug 12, 2024
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.

2 participants