Repository navigation
Refactor wrap_preserving_code into helper utilities - #233
Conversation
Reviewer's GuideRefactored wrap_preserving_code by extracting state management and control flow into dedicated helper utilities, simplifying the core loop and improving readability, and enhanced test coverage for each helper behavior. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughSummarise the refactor: replace ad-hoc inline wrapping with Unicode-aware width calculations and a LineBuffer-driven flow, add determine_token_span and attach_punctuation_to_previous_line, and update wrap_preserving_code and tests to use the new grouping and mid-wrap handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Wrap as wrap_preserving_code
participant Tokeniser as segment_inline
participant Line as LineBuffer
Caller->>Wrap: wrap_preserving_code(text, width)
Wrap->>Tokeniser: segment_inline(text)
Tokeniser-->>Wrap: tokens
loop tokens
Wrap->>Wrap: determine_token_span(tokens, idx)
alt attach end punctuation
Wrap->>Line: attach_punctuation_to_previous_line(group)
else push group to buffer
Wrap->>Line: push_group / push_non_whitespace_group(group)
alt group overflows
Wrap->>Line: flush_into(target_lines) %% handle mid-wrap split
end
end
end
Wrap-->>Caller: return wrapped_lines
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Consider moving LineBuffer and its associated helper functions into their own module or file to keep wrap.rs focused on high-level logic.
- The helper function names like try_attach_punctuation_to_previous_line and handle_trailing_whitespace_group are a bit verbose—consider simplifying or standardizing naming for better readability.
- wrap_preserving_code still has repeated patterns around creating and using LineBuffer—extracting that control flow into a single orchestrator function could reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving LineBuffer and its associated helper functions into their own module or file to keep wrap.rs focused on high-level logic.
- The helper function names like try_attach_punctuation_to_previous_line and handle_trailing_whitespace_group are a bit verbose—consider simplifying or standardizing naming for better readability.
- wrap_preserving_code still has repeated patterns around creating and using LineBuffer—extracting that control flow into a single orchestrator function could reduce duplication.
## Individual Comments
### Comment 1
<location> `src/wrap/tests.rs:15-25` </location>
<code_context>
+ wrap_preserving_code,
+};
+
+#[rstest]
+#[case("`code`!", "`code`!")]
+#[case("[link](url).", "[link](url).")]
+#[case("plain,", "plain,")]
+fn determine_token_span_groups_related_tokens(#[case] input: &str, #[case] expected_group: &str) {
+ let tokens = segment_inline(input);
+ let (end, width) = determine_token_span(&tokens, 0);
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding cases for edge punctuation and nested code/link tokens.
Please add tests for nested punctuation and tokens with adjacent punctuation or whitespace to ensure the grouping logic handles these edge cases.
```suggestion
#[rstest]
#[case("`code`!", "`code`!")]
#[case("[link](url).", "[link](url).")]
#[case("plain,", "plain,")]
// Edge punctuation and nested code/link tokens
#[case("`code`,", "`code`,")]
#[case("`code`!`more`", "`code`!`more`")]
#[case("[link](url),", "[link](url),")]
#[case("[link](url) [another](url2)", "[link](url) [another](url2)")]
#[case("`code` ,", "`code` ,")]
#[case("[link](url) .", "[link](url) .")]
#[case("`code!`", "`code!`")]
#[case("[link!](url)", "[link!](url)")]
fn determine_token_span_groups_related_tokens(#[case] input: &str, #[case] expected_group: &str) {
let tokens = segment_inline(input);
let (end, width) = determine_token_span(&tokens, 0);
let grouped = tokens[..end].join("");
assert_eq!(grouped, expected_group);
assert_eq!(width, unicode_width::UnicodeWidthStr::width(expected_group));
}
```
</issue_to_address>
### Comment 2
<location> `src/wrap/tests.rs:27-28` </location>
<code_context>
+ assert_eq!(width, unicode_width::UnicodeWidthStr::width(expected_group));
+}
+
+#[test]
+fn append_group_to_line_updates_last_split_for_whitespace() {
+ let tokens = segment_inline("foo bar");
+ let mut current = String::new();
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not cover multiple consecutive whitespace tokens.
Please add a test case with multiple consecutive whitespace tokens to verify correct `last_split` updates.
</issue_to_address>
### Comment 3
<location> `src/wrap.rs:192` </location>
<code_context>
+ }
+}
+
+fn handle_split_overflow(
+ lines: &mut Vec<String>,
+ buffer: &mut LineBuffer<'_>,
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining the overflow and trailing-whitespace helpers back into the main loop to avoid excessive indirection and improve readability.
You’ve now broken the one big loop into **seven** tiny functions and a `LineBuffer` wrapper—unfortunately that indirection makes the logic harder to follow. You can collapse **all** of the “overflow / trailing‐whitespace / new‐line” helpers back into the loop and keep only one small helper for finding `(end, width)`. This keeps the flow in one place while preserving every edge‐case.
For example, you can replace:
```rust
// … inside wrap_preserving_code …
let (group_end, group_width) = determine_token_span(&tokens, i);
if try_attach_punctuation_to_previous_line(…) { i = group_end; continue; }
if current_width + group_width <= width {
append_group_to_line(…);
i = group_end;
continue;
}
if handle_split_overflow(…) { i = group_end; continue; }
if handle_trailing_whitespace_group(…) { i = group_end; continue; }
start_new_line_with_group(…);
i = group_end;
```
with a single loop that inlines those two middle cases:
```rust
while i < tokens.len() {
// 1) figure out how big this “unit” is
let (end, group_w) = determine_token_span(&tokens, i);
// 2) single‐char punctuation glued to previous code line?
if current.is_empty()
&& tokens[i].len()==1
&& ".?!,:;".contains(&tokens[i])
&& lines.last().map_or(false, |l| l.trim_end().ends_with('`'))
{
lines.last_mut().unwrap().push_str(&tokens[i]);
i = end;
continue;
}
// 3) fits on the current line?
if current_width + group_w <= width {
for tok in &tokens[i..end] {
current.push_str(tok);
if tok.chars().all(char::is_whitespace) {
last_split = Some(current.len());
}
current_width += UnicodeWidthStr::width(tok);
}
i = end;
continue;
}
// 4) split at the last whitespace we recorded?
if let Some(pos) = last_split {
let head = current[..pos].trim_end().to_string();
if !head.is_empty() { lines.push(head); }
// rebuild remainder + this group
let mut tail = current[pos..].trim_start().to_string();
for tok in &tokens[i..end] { tail.push_str(tok); }
current = tail;
current_width = UnicodeWidthStr::width(¤t);
last_split = (tokens[end-1].chars().all(char::is_whitespace))
.then(|| current.len());
if current_width > width {
// overflowed even after split
lines.push(current.trim_end().to_string());
current.clear();
current_width = 0;
last_split = None;
}
i = end;
continue;
}
// 5) trailing whitespace at very end of input?
if tokens[i].chars().all(char::is_whitespace) && end == tokens.len() {
if !current.is_empty() {
flush_trailing_whitespace(&mut lines, &mut current, &tokens[i]);
}
current_width = 0;
last_split = None;
i = end;
continue;
}
// 6) otherwise, start a brand new line
if !current.is_empty() {
flush_current(&mut lines, &mut current);
}
current_width = 0;
last_split = None;
for tok in &tokens[i..end] {
if !tok.chars().all(char::is_whitespace) {
current.push_str(tok);
current_width += UnicodeWidthStr::width(tok);
}
}
if tokens[end-1].chars().all(char::is_whitespace) {
last_split = Some(current.len());
}
i = end;
}
```
By inlining the two “overflow” helpers (steps 4 and 5) you:
- remove `LineBuffer` entirely
- collapse `handle_split_overflow` + `handle_trailing_whitespace_group`
- keep everything in one readable loop
Functionality remains **identical** but the control-flow is now **visible in one place** again.
</issue_to_address>
### Comment 4
<location> `src/wrap.rs:144` </location>
<code_context>
+ }
+}
+
+fn determine_token_span(tokens: &[String], start: usize) -> (usize, usize) {
+ let mut end = start + 1;
+ let mut width = UnicodeWidthStr::width(tokens[start].as_str());
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new determine_token_span function.
This function is newly introduced and contains non-trivial logic for grouping tokens. You must add both behavioural and unit tests to verify its correctness, especially for edge cases.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `src/wrap.rs:163` </location>
<code_context>
+ (end, width)
+}
+
+fn try_attach_punctuation_to_previous_line(
+ lines: &mut [String],
+ current: &str,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for try_attach_punctuation_to_previous_line.
This helper function changes how punctuation is attached to previous lines. You must provide tests that demonstrate its behaviour in various scenarios.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 6
<location> `src/wrap.rs:182` </location>
<code_context>
+ false
+}
+
+fn append_group_to_line(tokens: &[String], start: usize, end: usize, buffer: &mut LineBuffer<'_>) {
+ for tok in &tokens[start..end] {
+ buffer.text.push_str(tok);
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for append_group_to_line.
This function is new and should be covered by both behavioural and unit tests to ensure correct token appending and whitespace handling.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 7
<location> `src/wrap.rs:192` </location>
<code_context>
+ }
+}
+
+fn handle_split_overflow(
+ lines: &mut Vec<String>,
+ buffer: &mut LineBuffer<'_>,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for handle_split_overflow.
This function handles line overflow and splitting. You must add tests to verify its behaviour, especially for edge cases and correctness of line splitting.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 8
<location> `src/wrap.rs:231` </location>
<code_context>
+ true
+}
+
+fn handle_trailing_whitespace_group(
+ lines: &mut Vec<String>,
+ buffer: &mut LineBuffer<'_>,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for handle_trailing_whitespace_group.
This function is new and should be covered by both behavioural and unit tests to ensure correct handling of trailing whitespace groups.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 9
<location> `src/wrap.rs:250` </location>
<code_context>
+ true
+}
+
+fn start_new_line_with_group(
+ lines: &mut Vec<String>,
+ buffer: &mut LineBuffer<'_>,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for start_new_line_with_group.
This function is new and should be covered by both behavioural and unit tests to ensure correct behaviour when starting new lines with token groups.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 10
<location> `src/wrap.rs:118` </location>
<code_context>
flush_current(lines, current);
}
+struct LineBuffer<'a> {
+ text: &'a mut String,
+ width: &'a mut usize,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new LineBuffer struct and its methods.
LineBuffer is a new struct with methods that affect line construction. You must add tests to verify its behaviour and correctness.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</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}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap.rs(2 hunks)src/wrap/tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/wrap.rssrc/wrap/tests.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/wrap.rssrc/wrap/tests.rs
🧬 Code graph analysis (2)
src/wrap.rs (1)
src/wrap/tokenize.rs (1)
segment_inline(173-233)
src/wrap/tests.rs (2)
src/wrap.rs (8)
append_group_to_line(182-190)determine_token_span(144-161)handle_split_overflow(192-229)handle_trailing_whitespace_group(231-248)start_new_line_with_group(250-274)wrap_preserving_code(276-325)new(125-131)is_empty(133-135)src/wrap/tokenize.rs (1)
segment_inline(173-233)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (6)
src/wrap/tests.rs (4)
15-25: LGTM: token grouping tests exercise key paths.Good coverage for code spans, links, and punctuation grouping. Width assertions match
unicode-width.
27-49: LGTM: whitespace updateslast_splitas intended.The assertions match the intended buffer semantics.
51-76: LGTM: overflow handling path validated.This verifies split-at-last-whitespace and remainder carry-over.
78-111: LGTM: trailing whitespace preservation covered.This nails the flush behaviour for hard breaks.
src/wrap.rs (2)
182-190: LGTM: append group updates width and split position correctly.Whitespace correctly updates
last_split; width usesunicode-width.
144-161: Edge‑case check: unmatched multi‑backtick runs.
determine_token_spanmerges only when the opener token is exactly "". Unmatched runs like "``" will not merge the remainder. Verify thatsegment_inline` never emits a bare "``" token without a matching closer when a single backtick would be more appropriate.Run this quick probe to find call sites where an unmatched backtick run longer than 1 might reach
determine_token_span:#!/bin/bash rg -n -C2 --type=rust "segment_inline\(" rg -n -C1 --type=rust "determine_token_span\("
Switch wrap_preserving_code to slice-based handling when rewinding at the last split so multi-space runs stay intact without cloning intermediate substrings. Update attach_punctuation_to_previous_line to use nested guards compatible with stable Rust.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap.rs(2 hunks)src/wrap/tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/wrap/tests.rssrc/wrap.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/wrap/tests.rssrc/wrap.rs
🧬 Code graph analysis (2)
src/wrap/tests.rs (2)
src/wrap.rs (8)
attach_punctuation_to_previous_line(128-143)determine_token_span(103-126)wrap_preserving_code(298-339)width(161-163)new(153-155)text(157-159)try_split_with_group(217-264)handle_trailing_whitespace_group(266-295)src/wrap/tokenize.rs (1)
segment_inline(173-233)
src/wrap.rs (1)
src/wrap/tokenize.rs (1)
segment_inline(173-233)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (13)
src/wrap/tests.rs (8)
14-34: Approve: Good coverage for grouped token spans.Tests exercise links, code spans, adjacent punctuation, and chained code tokens; assertions include width checks.
36-70: Approve: Punctuation attachment behaviour validated.Cases assert correct gating on empty current buffer and code-suffix requirement.
72-87: Approve: Trim trailing whitespace before gluing punctuation.The LineBuffer test matches the intended removal before appending punctuation.
89-104: Approve: Split preserves multi-space semantics.The split path keeps two+ trailing spaces on the emitted line and carries remainder correctly.
106-123: Approve: Trailing whitespace at end flushes correctly.The buffer flushes with trailing spaces on the final line and resets state.
124-135: Approve: Consecutive whitespace drives wrap boundaries.The expected three-line output demonstrates correct whitespace-driven splits.
137-141: Approve: Glue punctuation after code with intervening space.This confirms the in-buffer glue path independent of the “attach to previous line” helper.
9-12: Remove wildcard import and importwrap_textexplicitly.Drop
super::super::*and importwrap_textdirectly to keep scope tight and avoid shadowing.-use super::super::*; use super::{ LineBuffer, attach_punctuation_to_previous_line, determine_token_span, tokenize::segment_inline, wrap_preserving_code, }; +use super::wrap_text;As per coding guidelines
src/wrap.rs (5)
102-126: Approve: Token-span grouping is well‑defined.The logic groups links with trailing punctuation, handles matched code spans, and chains adjacent code tokens as intended; width accounting is Unicode‑aware.
128-143: Approve: Safe punctuation attach to previous line.Gate on empty current buffer, single‑char ASCII end punctuation, and code‑suffix on the last line. Early exits prevent accidental mutation.
165-187: Approve: Trim trailing whitespace before appending punctuation.The removal of trailing spaces when gluing punctuation after code is correct. Width and
last_splitupdates are consistent with the trimmed buffer.
266-295: Approve: Trailing whitespace handling at input end is correct.Only flush trailing whitespace when the buffer has content; otherwise no‑op. State resets are coherent.
298-339: Approve: Main wrapping loop is coherent and side‑effect free.The flow orders punctuation attach, in‑bounds append, split‑overflow, terminal whitespace handling, then flush/new‑line seeding. Buffer state transitions are consistent.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Stop carrying last_split markers when no whitespace is appended and trim split heads using slices to avoid extra allocations.
- reuse slice indices in LineBuffer::try_split_with_group so overflow heads and buffer tails avoid intermediate allocations while preserving whitespace semantics - extend LineBuffer split tests to cover single-space trims and width bookkeeping, and switch wrap tests to import wrap_text explicitly
Markdown renders a soft line break as a space. When `--wrap` breaks between two characters that touch in the source, it therefore adds a space to the rendered text, and two or more trailing spaces on a line it broke render as a hard break. #561 reported five such shapes: `(` before a reference link, `/` between code spans, `-[` inside a word, `**` around a code span, and the mirror `][4]).`. It also reported a break at a run of two spaces. After grouping, `join_touching_fragments` merges every pair of neighbouring fragments whose seam has no whitespace, so whitespace fragments are the only places `textwrap` can break. `render_line` now trims every trailing space from a line the wrapper broke; the final line keeps the source's own. The same mechanism fixes #507. The shapes regressed in #271 (the textwrap adapter), #395 (reference links became one token) and #233 (which pinned the hard break in `wrap_preserving_code_splits_after_consecutive_whitespace`). That test is restored to expect no trailing spaces. The `spanning_code_span_-` snapshot is corrected, since it recorded a break inside `` kani-full` ``. `tests/issue_561_wrap_never_breaks_without_whitespace.rs` pins each reproduction and checks the word-preservation invariant as a property.
Break wrapped lines only at source whitespace (#561) `--wrap` could break inside a run of fragments with no whitespace between them, such as `([ref][4])`, `` `a`/`b` ``, `-[r]->` or `**`code`**`, adding a space to the rendered text, and a break at a run of spaces could leave two trailing spaces that render as a hard break. - Join touching fragments into one unbreakable run before fitting, in linear time, so the only break opportunities are source whitespace. - `render_line` trims every trailing space from a line the wrapper broke; only a segment's final line keeps the source's own. - Regression tests named after the issue, with idempotence checks and a CLI snapshot; the architecture, ADR 0002 addendum, developers' guide and CHANGELOG record the rule. Regressed in #271 (2de1134), #395 (6758894) and #233 (822f964).
Summary
LineBufferhelper and other utilities sowrap_preserving_codeno longer manages every concern inlineTesting
https://chatgpt.com/codex/tasks/task_e_68d502e4898c832288984f8bc44f8d70
Summary by Sourcery
Refactor wrap_preserving_code by extracting line buffering and token‐grouping logic into dedicated helper functions to simplify wrapping control flow and punctuation/whitespace handling, and add targeted unit tests for each helper.
Enhancements:
Tests: