Conversation
Reviewer's GuideRefactor wrap_text to treat trailing backslashes as hard line breaks by reordering trimming logic and adding detection for odd backslash escapes, and introduce regression tests with fixtures for backslash hard breaks. Class diagram for updated wrap_text logicclassDiagram
class wrap_text {
+wrap_text(lines: &[String], width: usize) -> Vec<String>
-indent: String
-trimmed_end: String
-text_without_html_breaks: String
-is_trailing_spaces: bool
-is_html_br: bool
-backslash_count: usize
-is_backslash_escape: bool
-hard_break: bool
-text: String
}
wrap_text : +Detects hard breaks via trailing spaces, HTML <br>, or odd trailing backslash
wrap_text : +Regression test for backslash hard breaks added
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughRefactor the logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant FS as File System
participant Wrap as wrap_text
participant Proc as process_stream
Test->>FS: Read hard_linebreak_input.txt
Test->>FS: Read hard_linebreak_expected.txt
Test->>Proc: Call process_stream(input_lines)
Proc->>Wrap: For each line, call wrap_text(line)
Wrap-->>Proc: Return (text, hard_break)
Proc-->>Test: Return output_lines
Test->>Test: Assert output_lines == expected_lines
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rsInstructions used from: Sources: 🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/wrap.rs:323` </location>
<code_context>
+ text = text.trim_end_matches(' ').to_string();
+ }
+
+ if text.ends_with('\\') && !text.ends_with("\\\\") {
+ hard_break = true;
+ }
</code_context>
<issue_to_address>
The logic for detecting single trailing backslashes may not handle all edge cases.
Consider counting the number of trailing backslashes and checking if it is odd, to ensure all intended cases are handled.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if text.ends_with('\\') && !text.ends_with("\\\\") {
hard_break = true;
}
=======
// Count the number of trailing backslashes
let mut backslash_count = 0;
for c in text.chars().rev() {
if c == '\\' {
backslash_count += 1;
} else {
break;
}
}
if backslash_count % 2 == 1 {
hard_break = true;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tests/integration.rs:798` </location>
<code_context>
}
+#[test]
+fn test_wrap_hard_linebreak_backslash() {
+ let input: Vec<String> = include_lines!("data/hard_linebreak_input.txt");
+ let expected: Vec<String> = include_lines!("data/hard_linebreak_expected.txt");
+ assert_eq!(process_stream(&input), expected);
+}
+
</code_context>
<issue_to_address>
Consider adding edge cases for escaped backslashes and lines with only a backslash.
Please add tests for lines ending with double backslashes, lines with only a single backslash, and lines with whitespace around the backslash to ensure correct handling of these cases.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[test]
fn test_wrap_hard_linebreak_backslash() {
let input: Vec<String> = include_lines!("data/hard_linebreak_input.txt");
let expected: Vec<String> = include_lines!("data/hard_linebreak_expected.txt");
assert_eq!(process_stream(&input), expected);
}
=======
#[test]
fn test_wrap_hard_linebreak_backslash() {
let input: Vec<String> = include_lines!("data/hard_linebreak_input.txt");
let expected: Vec<String> = include_lines!("data/hard_linebreak_expected.txt");
assert_eq!(process_stream(&input), expected);
}
#[test]
fn test_wrap_hard_linebreak_double_backslash() {
let input = vec![
String::from("This line ends with two backslashes: \\\\"),
String::from("This line ends with a single backslash: \\"),
String::from(" \\ "),
String::from("\\"),
String::from("Text before \\ and after"),
String::from(" \\"),
String::from(""),
];
// Adjust the expected output according to the actual behavior of process_stream.
// These are placeholder expectations and may need to be updated.
let expected = vec![
String::from("This line ends with two backslashes: \\\\"),
String::from("This line ends with a single backslash: \\"),
String::from(" \\ "),
String::from("\\"),
String::from("Text before \\ and after"),
String::from(" \\"),
String::from(""),
];
assert_eq!(process_stream(&input), expected);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/wrap.rs:304` </location>
<code_context>
- || trimmed_end.ends_with("<br/>")
- || trimmed_end.ends_with("<br />");
- let text = trimmed_end
+ let mut hard_break = false;
+ let mut text = trimmed_end
.trim_end_matches("<br>")
</code_context>
<issue_to_address>
Consider combining all hard-break conditions into immutable variables and chaining trims before a single to_string() call.
```rust
// Combine all hard‐break conditions into one immutable boolean,
// and chain your trims before the single `to_string()` call:
let trimmed_end = line.trim_end();
let is_trailing_spaces = line.ends_with(" ");
let is_html_br = trimmed_end.ends_with("<br>")
|| trimmed_end.ends_with("<br/>")
|| trimmed_end.ends_with("<br />");
let is_backslash_escape = trimmed_end.ends_with('\\')
&& !trimmed_end.ends_with("\\\\");
let hard_break = is_trailing_spaces || is_html_br || is_backslash_escape;
let text = trimmed_end
.trim_start()
.trim_end_matches("<br>")
.trim_end_matches("<br/>")
.trim_end_matches("<br />")
.trim_end_matches(' ')
.to_string();
buf.push((text, hard_break));
```
Steps:
1. Detect each break‐condition in its own `let` for clarity.
2. OR them into a single `hard_break` – no `mut` needed.
3. Apply all `trim_*` calls on `trimmed_end` first, then call `to_string()` exactly once.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/wrap.rs (1)
309-323: Consider the past review suggestion for cleaner variable organisation.The current implementation works correctly but could benefit from the structure suggested in past reviews. Combine all hard-break conditions into immutable variables for better clarity.
- let is_trailing_spaces = line.ends_with(" "); - let is_html_br = trimmed_end != without_br; - - // Count trailing backslashes to handle escaped pairs correctly - let mut backslashes = 0; - for ch in without_br.chars().rev() { - if ch == '\\' { - backslashes += 1; - } else { - break; - } - } - let is_backslash_escape = backslashes % 2 == 1; - - let hard_break = is_trailing_spaces || is_html_br || is_backslash_escape; + let is_trailing_spaces = line.ends_with(" "); + let is_html_br = trimmed_end != without_br; + let is_backslash_escape = { + let mut backslashes = 0; + for ch in without_br.chars().rev() { + if ch == '\\' { + backslashes += 1; + } else { + break; + } + } + backslashes % 2 == 1 + }; + let hard_break = is_trailing_spaces || is_html_br || is_backslash_escape;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap.rs(1 hunks)tests/integration.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧬 Code Graph Analysis (1)
tests/integration.rs (1)
src/process.rs (1)
process_stream(76-76)
🔇 Additional comments (5)
src/wrap.rs (3)
312-321: The backslash counting logic correctly handles escaped pairs.The implementation properly counts trailing backslashes and uses modulo arithmetic to determine if there's an odd number, which correctly identifies unescaped backslashes that should trigger hard breaks. This addresses the edge case mentioned in past reviews.
304-307: Simplify the chained trim operations.Chain the trim operations in a single expression rather than spreading across multiple lines for better readability.
- let without_br = trimmed_end - .trim_end_matches("<br>") - .trim_end_matches("<br/>") - .trim_end_matches("<br />"); + let without_br = trimmed_end + .trim_end_matches("<br>") + .trim_end_matches("<br/>") + .trim_end_matches("<br />");Likely an incorrect or invalid review comment.
325-325: Verify trimming preserves backslashesEnsure the sequence
trim_start().trim_end_matches(' ')removes only whitespace and does not strip backslashes at the edges.• Location: src/wrap.rs:325
• Current snippet:let text = without_br.trim_start().trim_end_matches(' ').to_string();Add unit tests covering cases such as:
- Leading whitespace with backslashes:
" \\example"- Trailing spaces after backslashes:
"example\\ "- Combined leading/trailing whitespace and backslashes
Validate that backslashes remain intact while only extra spaces are removed.
tests/integration.rs (2)
797-802: Approve the file-based test approach.Using external files for input and expected output provides good separation of test data and allows for complex real-world scenarios to be tested effectively.
804-826: The edge case test covers comprehensive backslash scenarios.The test appropriately covers the edge cases mentioned in past reviews:
- Double backslashes (escaped, should not trigger hard break)
- Single backslashes (unescaped, should trigger hard break)
- Whitespace around backslashes
- Lines containing only backslashes
The expected output correctly demonstrates the merging behaviour where single backslashes cause line continuation.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/wrap.rs (1)
314-314: Apply the immutable variable suggestion from past reviews.Combine all hard-break conditions as suggested in the past review comments to improve code clarity and follow the immutable data preference from coding guidelines.
tests/integration.rs (1)
804-824: Replace inline test with rstest parameterisation.Follow the coding guideline to use rstest for parameterised test cases rather than inline vectors. This improves test organisation and makes individual scenarios easier to identify.
Apply the refactoring suggested in the past review comments to use rstest parameterisation instead of inline vectors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap.rs(1 hunks)tests/integration.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🧬 Code Graph Analysis (1)
tests/integration.rs (1)
src/process.rs (1)
process_stream(76-76)
🔇 Additional comments (1)
tests/integration.rs (1)
797-802: Excellent test implementation using external files.This test follows best practices by using external files for input and expected data, making it easy to maintain and understand.
| let expected = lines_vec!( | ||
| "This line ends with two backslashes: \\\\ This line ends with a single backslash:", | ||
| "\\", | ||
| "\\", | ||
| "\\", | ||
| "Text before \\ and after \\", | ||
| "", | ||
| ); | ||
| assert_eq!(process_stream(&input), expected); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the expected test outputs match the implementation.
The expected outputs suggest specific behaviour for backslash line breaks. Ensure these expectations align with the actual implementation in src/wrap.rs.
🏁 Script executed:
#!/bin/bash
# Description: Verify the backslash line break behaviour matches test expectations
# Expected: Lines with odd trailing backslashes should create hard breaks
# Test the actual behaviour with the edge case inputs
echo "Testing backslash line break behaviour..."
# Check if the wrap_text function properly handles these cases
ast-grep --pattern 'fn wrap_text($_, $_) -> $_ {
$$$
}'Length of output: 152
🏁 Script executed:
#!/bin/bash
# Locate process_stream implementation
rg -n "fn process_stream"
# Locate wrap_text implementation
rg -n "fn wrap_text" || echo "No direct wrap_text; searching usage"
rg -n "wrap_text"
# Find the wrap.rs file
WRAP_FILE=$(find src -name wrap.rs | head -n1)
echo "wrap.rs found at: $WRAP_FILE"
# Display the first 200 lines of wrap.rs for context
sed -n '1,200p' "$WRAP_FILE"Length of output: 8807
🏁 Script executed:
#!/bin/bash
# Display integration test around backslash behaviour
echo "=== Integration Test Snippet ==="
sed -n '780,860p' tests/integration.rs
# Display process_stream_inner implementation to inspect hard-break logic
echo -e "\n=== process.rs (lines 1–200) ==="
sed -n '1,200p' src/process.rsLength of output: 5645
🏁 Script executed:
#!/bin/bash
echo "=== wrap_text definition ==="
sed -n '200,360p' src/wrap.rs
echo -e "\n=== hard_break detection ==="
grep -n "hard_break" -n src/wrap.rsLength of output: 5434
Update backslash edge-case test expected outputs
Adjust the test_wrap_hard_linebreak_backslash_edge_cases expectations to match the actual process_stream behaviour in wrap.rs:
• File: tests/integration.rs
• Test: test_wrap_hard_linebreak_backslash_edge_cases
• Replace the expected vector with this diff:
-let expected = lines_vec!(
- "This line ends with two backslashes: \\\\ This line ends with a single backslash:",
- "\\",
- "\\",
- "\\",
- "Text before \\ and after \\",
- "",
-);
+let expected = lines_vec!(
+ "This line ends with two backslashes: \\\\ This line ends with a single backslash:\\",
+ "\\",
+ "\\",
+ "Text before \\ and after \\",
+ "",
+);This removes the extra lone backslash line and preserves the trailing backslash on the first combined line.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let expected = lines_vec!( | |
| "This line ends with two backslashes: \\\\ This line ends with a single backslash:", | |
| "\\", | |
| "\\", | |
| "\\", | |
| "Text before \\ and after \\", | |
| "", | |
| ); | |
| assert_eq!(process_stream(&input), expected); | |
| let expected = lines_vec!( | |
| - "This line ends with two backslashes: \\\\ This line ends with a single backslash:", | |
| - "\\", | |
| - "\\", | |
| - "\\", | |
| - "Text before \\ and after \\", | |
| - "", | |
| + "This line ends with two backslashes: \\\\ This line ends with a single backslash:\\", | |
| + "\\", | |
| + "\\", | |
| + "Text before \\ and after \\", | |
| + "", | |
| ); | |
| assert_eq!(process_stream(&input), expected); |
🤖 Prompt for AI Agents
In tests/integration.rs around lines 815 to 823, the expected output in the
test_wrap_hard_linebreak_backslash_edge_cases test does not match the actual
behavior of process_stream in wrap.rs. Update the expected vector by removing
the extra lone backslash lines and ensure the trailing backslash is preserved on
the first combined line, so the expected output aligns with the real output of
process_stream.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/wrap.rs (1)
304-320: Address the backslash detection logic flaw identified in previous reviews.The current implementation still has the same logical issue previously identified. You're counting backslashes in
trimmed_end(line 311) but should be counting them before any HTML processing occurs. A line ending with\<br>should not be treated as a backslash hard break.The variable naming has been improved to
text_without_html_breaksas suggested, but the core logic flaw remains unaddressed.Apply this fix to correct the backslash detection:
- let backslash_count = trimmed_end.chars().rev().take_while(|&c| c == '\\').count(); + let backslash_count = line.trim_end().chars().rev().take_while(|&c| c == '\\').count();This ensures backslashes are counted from the original line content before any HTML tag processing, preventing false positives when a line ends with
\<br>.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/wrap.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🔇 Additional comments (1)
src/wrap.rs (1)
316-319: Verify text processing consistency.The text processing now correctly uses
text_without_html_breaksas the source, which addresses the previous review feedback. This ensures consistency between the break detection logic and the final text output.The trimming operations are applied in the correct order: HTML breaks are removed first, then leading/trailing whitespace is handled appropriately.
| let is_trailing_spaces = line.ends_with(" "); | ||
| let is_html_br = trimmed_end != text_without_html_breaks; | ||
| let backslash_count = trimmed_end.chars().rev().take_while(|&c| c == '\\').count(); | ||
| let is_backslash_escape = backslash_count % 2 == 1; | ||
|
|
||
| let hard_break = is_trailing_spaces || is_html_br || is_backslash_escape; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify the hard break detection logic for better maintainability.
The current implementation correctly separates the different hard break conditions, but the logic can be streamlined. The approach of checking each condition separately is good for clarity, but ensure all edge cases are properly handled.
The logic is functionally correct after fixing the backslash detection source. The separation of concerns between trailing spaces, HTML breaks, and backslash escapes makes the code more maintainable than the previous implementation.
🤖 Prompt for AI Agents
In src/wrap.rs around lines 309 to 314, simplify the hard break detection by
consolidating the conditions into a clearer, more maintainable expression. Keep
the separate checks for trailing spaces, HTML breaks, and backslash escapes for
clarity, but refactor the code to reduce redundancy and improve readability
while ensuring all edge cases remain correctly handled.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@sourcery-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/integration.rs:806` </location>
<code_context>
+
+#[test]
+fn test_wrap_hard_linebreak_backslash_edge_cases() {
+ let input = lines_vec!(
+ "This line ends with two backslashes: \\\\",
+ "This line ends with a single backslash: \\",
+ " \\ ",
+ "\\",
+ "Text before \\ and after",
+ " \\",
+ "",
+ );
+ let expected = lines_vec!(
+ "This line ends with two backslashes: \\\\ This line ends with a single backslash:",
</code_context>
<issue_to_address>
Consider adding a test case for a line ending with an odd number of backslashes greater than one.
Please add a test case with a line ending in three backslashes to ensure correct handling of odd numbers of trailing backslashes.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let input = lines_vec!(
"This line ends with two backslashes: \\\\",
"This line ends with a single backslash: \\",
" \\ ",
"\\",
"Text before \\ and after",
" \\",
"",
);
let expected = lines_vec!(
"This line ends with two backslashes: \\\\ This line ends with a single backslash:",
"\\",
"\\",
"\\",
"Text before \\ and after \\",
"",
);
=======
let input = lines_vec!(
"This line ends with two backslashes: \\\\",
"This line ends with a single backslash: \\",
"This line ends with three backslashes: \\\\\\",
" \\ ",
"\\",
"Text before \\ and after",
" \\",
"",
);
let expected = lines_vec!(
"This line ends with two backslashes: \\\\ This line ends with a single backslash:",
"This line ends with three backslashes: \\\\\\",
"\\",
"\\",
"\\",
"Text before \\ and after \\",
"",
);
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let input = lines_vec!( | ||
| "This line ends with two backslashes: \\\\", | ||
| "This line ends with a single backslash: \\", | ||
| " \\ ", | ||
| "\\", | ||
| "Text before \\ and after", | ||
| " \\", | ||
| "", | ||
| ); | ||
| let expected = lines_vec!( | ||
| "This line ends with two backslashes: \\\\ This line ends with a single backslash:", | ||
| "\\", | ||
| "\\", | ||
| "\\", | ||
| "Text before \\ and after \\", | ||
| "", | ||
| ); |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for a line ending with an odd number of backslashes greater than one.
Please add a test case with a line ending in three backslashes to ensure correct handling of odd numbers of trailing backslashes.
| let input = lines_vec!( | |
| "This line ends with two backslashes: \\\\", | |
| "This line ends with a single backslash: \\", | |
| " \\ ", | |
| "\\", | |
| "Text before \\ and after", | |
| " \\", | |
| "", | |
| ); | |
| let expected = lines_vec!( | |
| "This line ends with two backslashes: \\\\ This line ends with a single backslash:", | |
| "\\", | |
| "\\", | |
| "\\", | |
| "Text before \\ and after \\", | |
| "", | |
| ); | |
| let input = lines_vec!( | |
| "This line ends with two backslashes: \\\\", | |
| "This line ends with a single backslash: \\", | |
| "This line ends with three backslashes: \\\\\\", | |
| " \\ ", | |
| "\\", | |
| "Text before \\ and after", | |
| " \\", | |
| "", | |
| ); | |
| let expected = lines_vec!( | |
| "This line ends with two backslashes: \\\\ This line ends with a single backslash:", | |
| "This line ends with three backslashes: \\\\\\", | |
| "\\", | |
| "\\", | |
| "\\", | |
| "Text before \\ and after \\", | |
| "", | |
| ); |
1 participant
Summary
\line breaksTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68796fd312088322ad7527eee86abd9e
Summary by Sourcery
Treat trailing backslashes as hard line breaks in text wrapping and add regression tests to validate the new behavior
New Features:
Enhancements:
tags, or odd-count backslashes
Tests: