Fix async attribute wrapping with punctuation#85
Conversation
Reviewer's GuideThis pull request refines the text wrapping logic to ensure that closing punctuation is not separated from inline code spans, and adds a regression test to verify correct handling of async attribute syntax with punctuation. Class diagram for test updates in wrap.rsclassDiagram
class tests {
+test_wrap_preserving_code() // updated expected output for punctuation handling
+test_dangling_code_span() // updated expected output for inline code and punctuation
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughUpdate the internal line-wrapping logic in Changes
Sequence Diagram(s)sequenceDiagram
participant Input
participant wrap_preserving_code
participant Output
Input->>wrap_preserving_code: Provide tokens for a line
loop For each token
wrap_preserving_code->>wrap_preserving_code: Check if token fits in current line
alt Token fits
alt Token is whitespace
wrap_preserving_code->>wrap_preserving_code: Update last_split position
end
wrap_preserving_code->>wrap_preserving_code: Append token to buffer
else Token does not fit
alt last_split exists
wrap_preserving_code->>Output: Push line up to last_split
wrap_preserving_code->>wrap_preserving_code: Start new buffer with remainder and current token
else
wrap_preserving_code->>Output: Push current buffer as line
wrap_preserving_code->>wrap_preserving_code: Start new buffer with current token
end
end
end
wrap_preserving_code->>Output: Push any remaining buffer as line
Possibly related issues
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:
⚙️ CodeRabbit Configuration File 🔇 Additional comments (2)
✨ 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}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap.rs(3 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 (3)
src/wrap.rs (2)
357-358: LGTM - test correctly validates improved code span preservation.The updated expected output properly reflects the new whitespace-aware wrapping behavior, ensuring inline code spans remain intact across line breaks.
395-395: LGTM - test correctly handles unmatched backtick preservation.The updated assertion properly validates that unmatched backticks and following text move together to the next line, maintaining the integrity of code-like tokens.
tests/integration.rs (1)
739-759: LGTM - comprehensive integration test for the async attribute wrapping fix.This test effectively validates the core fix described in the PR objectives. It ensures that Rust attribute syntax in inline code spans (
#[awt],#[future(awt)]) and their associated punctuation are preserved together during line wrapping, preventing the formatting issues that prompted this fix.
| let mut last_split: Option<usize> = None; | ||
| for token in tokenize_markdown(text) { | ||
| let token_width = UnicodeWidthStr::width(token.as_str()); | ||
| if current_width + token_width <= width { | ||
| current.push_str(&token); | ||
| current_width += token_width; | ||
| if token.chars().all(char::is_whitespace) { | ||
| last_split = Some(current.len()); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if let Some(pos) = last_split { | ||
| let line = current[..pos].to_string(); | ||
| let mut rest = current[pos..].trim_start().to_string(); | ||
| let trimmed = line.trim_end(); | ||
| if !trimmed.is_empty() { | ||
| lines.push(trimmed.to_string()); | ||
| } | ||
| rest.push_str(&token); | ||
| current = rest; | ||
| current_width = UnicodeWidthStr::width(current.as_str()); | ||
| last_split = if token.chars().all(char::is_whitespace) { | ||
| Some(current.len()) | ||
| } else { | ||
| None | ||
| }; | ||
| if current_width > width { | ||
| lines.push(current.trim_end().to_string()); | ||
| current.clear(); | ||
| current_width = 0; | ||
| last_split = None; | ||
| } | ||
| continue; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refactor the complex branching logic to improve readability.
The whitespace-aware wrapping implementation is correct, but the nested conditions and multiple control flow paths increase cyclomatic complexity. Extract the line-breaking logic into a separate predicate function to align with the coding guidelines.
+fn attempt_whitespace_break(
+ current: &mut String,
+ current_width: &mut usize,
+ last_split: &mut Option<usize>,
+ token: &str,
+ width: usize,
+ lines: &mut Vec<String>,
+) -> bool {
+ if let Some(pos) = *last_split {
+ let line = current[..pos].to_string();
+ let mut rest = current[pos..].trim_start().to_string();
+ let trimmed = line.trim_end();
+ if !trimmed.is_empty() {
+ lines.push(trimmed.to_string());
+ }
+ rest.push_str(token);
+ *current = rest;
+ *current_width = UnicodeWidthStr::width(current.as_str());
+ *last_split = if token.chars().all(char::is_whitespace) {
+ Some(current.len())
+ } else {
+ None
+ };
+ if *current_width > width {
+ lines.push(current.trim_end().to_string());
+ current.clear();
+ *current_width = 0;
+ *last_split = None;
+ }
+ true
+ } else {
+ false
+ }
+}Then simplify the main logic:
if current_width + token_width <= width {
current.push_str(&token);
current_width += token_width;
if token.chars().all(char::is_whitespace) {
last_split = Some(current.len());
}
continue;
}
- if let Some(pos) = last_split {
- let line = current[..pos].to_string();
- let mut rest = current[pos..].trim_start().to_string();
- let trimmed = line.trim_end();
- if !trimmed.is_empty() {
- lines.push(trimmed.to_string());
- }
- rest.push_str(&token);
- current = rest;
- current_width = UnicodeWidthStr::width(current.as_str());
- last_split = if token.chars().all(char::is_whitespace) {
- Some(current.len())
- } else {
- None
- };
- if current_width > width {
- lines.push(current.trim_end().to_string());
- current.clear();
- current_width = 0;
- last_split = None;
- }
+ if attempt_whitespace_break(&mut current, &mut current_width, &mut last_split, &token, width, &mut lines) {
continue;
}🤖 Prompt for AI Agents
In src/wrap.rs between lines 79 and 112, the current whitespace-aware wrapping
logic has complex nested conditions that reduce readability. Refactor by
extracting the line-breaking decision logic into a separate predicate function
that determines when to break lines based on width and whitespace. Then simplify
the main loop to call this function, reducing cyclomatic complexity and
improving clarity while preserving the existing behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/wrap.rs (1)
95-129: Approve the whitespace-aware wrapping implementation.The implementation correctly addresses the PR objective by preferring to split lines at whitespace boundaries rather than mid-token. The logic properly tracks the last whitespace position and attempts to break there when a token would exceed the width limit.
Note: The previous review comment about refactoring complex branching logic has been addressed by introducing the
should_break_linehelper function, which reduces the cyclomatic complexity of the main logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/wrap.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🪛 GitHub Actions: CI
src/wrap.rs
[error] 80-85: Rust doctest failed: function should_break_line is private and cannot be accessed in the test. Error E0603 at src/wrap.rs:82:23.
🔇 Additional comments (2)
src/wrap.rs (2)
374-375: Verify the test case update reflects the new wrapping behaviour.The test case has been updated to show that
`powershell -Command`now moves entirely to the next line rather than being split mid-token. This change aligns with the improved whitespace-aware wrapping logic.
412-412: Verify the unmatched backticks test reflects the new wrapping behaviour.The test case has been updated to show that
`danglingnow moves entirely to the next line with the following text, rather than splitting awkwardly. This demonstrates the improved preservation of inline code spans.
| /// Determine if the current line should break at the last whitespace. | ||
| /// | ||
| /// Returns `true` if `current_width` exceeds `width` and a whitespace split | ||
| /// position is available. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// use mdtablefix::wrap::should_break_line; | ||
| /// assert!(should_break_line(10, 12, Some(3))); | ||
| /// assert!(!should_break_line(10, 8, Some(3))); | ||
| /// ``` | ||
| fn should_break_line(width: usize, current_width: usize, last_split: Option<usize>) -> bool { | ||
| current_width > width && last_split.is_some() | ||
| } |
There was a problem hiding this comment.
Fix the doctest visibility issue and remove the example.
The pipeline failure indicates that should_break_line is private and cannot be accessed in the doctest. Additionally, providing a doctest example for an internal helper function violates the principle of documenting only public APIs as specified in the coding guidelines.
Apply this diff to fix the issue:
-/// Determine if the current line should break at the last whitespace.
-///
-/// Returns `true` if `current_width` exceeds `width` and a whitespace split
-/// position is available.
-///
-/// # Examples
-///
-/// ```
-/// use mdtablefix::wrap::should_break_line;
-/// assert!(should_break_line(10, 12, Some(3)));
-/// assert!(!should_break_line(10, 8, Some(3)));
-/// ```
+/// Determine if the current line should break at the last whitespace.
+///
+/// Returns `true` if `current_width` exceeds `width` and a whitespace split
+/// position is available.
fn should_break_line(width: usize, current_width: usize, last_split: Option<usize>) -> bool {
current_width > width && last_split.is_some()
}📝 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.
| /// Determine if the current line should break at the last whitespace. | |
| /// | |
| /// Returns `true` if `current_width` exceeds `width` and a whitespace split | |
| /// position is available. | |
| /// | |
| /// # Examples | |
| /// | |
| /// ``` | |
| /// use mdtablefix::wrap::should_break_line; | |
| /// assert!(should_break_line(10, 12, Some(3))); | |
| /// assert!(!should_break_line(10, 8, Some(3))); | |
| /// ``` | |
| fn should_break_line(width: usize, current_width: usize, last_split: Option<usize>) -> bool { | |
| current_width > width && last_split.is_some() | |
| } | |
| /// Determine if the current line should break at the last whitespace. | |
| /// | |
| /// Returns `true` if `current_width` exceeds `width` and a whitespace split | |
| /// position is available. | |
| fn should_break_line(width: usize, current_width: usize, last_split: Option<usize>) -> bool { | |
| current_width > width && last_split.is_some() | |
| } |
🧰 Tools
🪛 GitHub Actions: CI
[error] 80-85: Rust doctest failed: function should_break_line is private and cannot be accessed in the test. Error E0603 at src/wrap.rs:82:23.
🤖 Prompt for AI Agents
In src/wrap.rs around lines 73 to 87, the doctest for the private function
should_break_line causes pipeline failure because the function is not accessible
in tests and internal helpers should not have public examples. Remove the
doctest code block entirely, including the use statement and assert examples,
leaving only the function documentation comments describing its behavior.
1 participant
Summary
#[awt]and#[future]wordingTesting
make fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_68784f200da083228290b49a660bbeae
Summary by Sourcery
Improve markdown line wrapping logic to handle inline code and punctuation correctly, and add tests to prevent regressions with async attribute formatting.
Bug Fixes:
Tests: