Conversation
Reviewer's GuideRefactor wrap_text to extract detection and handling of list items, footnotes, and blockquotes into dedicated helper functions, streamlining the main loop. Class diagram for refactored wrap_text helpersclassDiagram
class wrap_text {
+wrap_text(lines: &[String], width: usize) -> Vec<String>
}
class handle_list_item {
+handle_list_item(out: &mut Vec<String>, buf: &mut Vec<(String, bool)>, indent: &mut String, width: usize, line: &str) -> bool
}
class handle_footnote {
+handle_footnote(out: &mut Vec<String>, buf: &mut Vec<(String, bool)>, indent: &mut String, width: usize, line: &str) -> bool
}
class handle_blockquote {
+handle_blockquote(out: &mut Vec<String>, buf: &mut Vec<(String, bool)>, indent: &mut String, width: usize, line: &str) -> bool
}
class is_list_item {
+is_list_item(line: &str) -> Option<(&str, &str)>
}
class is_footnote {
+is_footnote(line: &str) -> Option<(String, &str)>
}
class is_blockquote {
+is_blockquote(line: &str) -> Option<(&str, &str)>
}
wrap_text --> handle_list_item
wrap_text --> handle_footnote
wrap_text --> handle_blockquote
handle_list_item --> is_list_item
handle_footnote --> is_footnote
handle_blockquote --> is_blockquote
Flow diagram for wrap_text main loop after refactorflowchart TD
A[Start wrap_text main loop] --> B{Is line a list item?}
B -- Yes --> C[handle_list_item]
B -- No --> D{Is line a footnote?}
D -- Yes --> E[handle_footnote]
D -- No --> F{Is line a blockquote?}
F -- Yes --> G[handle_blockquote]
F -- No --> H[Other processing]
C --> I[Continue]
E --> I
G --> I
H --> I
I[Next line or end]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughRefactor the Changes
Sequence Diagram(s)sequenceDiagram
participant wrap_text
participant HANDLERS
participant handle_prefix_line
loop For each line in input
wrap_text ->> HANDLERS: Iterate over PrefixHandler array
alt Prefix matches
HANDLERS ->> wrap_text: Provide regex, prefix, and metadata
wrap_text ->> handle_prefix_line: Call with matched prefix and line
handle_prefix_line -->> wrap_text: Processed line
wrap_text -->> wrap_text: Continue to next line
else No prefix matches
wrap_text -->> wrap_text: Process as normal line
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
🔇 Additional comments (4)
✨ 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 - here's some feedback:
- There’s a lot of boilerplate across handle_list_item, handle_footnote, and handle_blockquote—consider introducing a single generic handler that takes a detection function and wrap flag to reduce duplication.
- The is_footnote helper allocates a new String for the prefix, unlike the other helpers—consider returning &str slices and composing the prefix later to avoid the extra allocation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of boilerplate across handle_list_item, handle_footnote, and handle_blockquote—consider introducing a single generic handler that takes a detection function and wrap flag to reduce duplication.
- The is_footnote helper allocates a new String for the prefix, unlike the other helpers—consider returning &str slices and composing the prefix later to avoid the extra allocation.
## Individual Comments
### Comment 1
<location> `src/wrap.rs:358` </location>
<code_context>
append_wrapped_with_prefix(out, prefix, rest, width, repeat_prefix);
}
+fn handle_list_item(
+ out: &mut Vec<String>,
+ buf: &mut Vec<(String, bool)>,
</code_context>
<issue_to_address>
Consider replacing the multiple is_* and handle_* helper functions with a single table-driven loop using a static handler array.
Here’s one way to collapse all six of those tiny helpers into a single, table-driven loop. You get rid of `is_*`/`handle_*` and keep _all_ of your existing behavior:
```rust
// Define once up front
struct PrefixHandler {
re: &'static Regex,
is_bq: bool,
build_prefix: fn(&Captures) -> String,
rest_group: usize,
}
static HANDLERS: &[PrefixHandler] = &[
PrefixHandler {
re: &BULLET_RE,
is_bq: false,
build_prefix: |cap| cap[1].to_string(),
rest_group: 2,
},
PrefixHandler {
re: &FOOTNOTE_RE,
is_bq: false,
build_prefix: |cap| format!("{}{}", &cap[1], &cap[2]),
rest_group: 3,
},
PrefixHandler {
re: &BLOCKQUOTE_RE,
is_bq: true,
build_prefix: |cap| cap[1].to_string(),
rest_group: 2,
},
];
```
Then in your loop in `wrap_text`:
```rust
'line_loop: for line in lines {
// ... blank‐line and code‐block logic ...
for handler in HANDLERS {
if let Some(cap) = handler.re.captures(line) {
let prefix = (handler.build_prefix)(&cap);
let rest = cap.get(handler.rest_group).unwrap().as_str();
handle_prefix_line(
&mut out,
&mut buf,
&mut indent,
width,
&prefix,
rest,
handler.is_bq,
);
continue 'line_loop;
}
}
// ... fall-through paragraph logic ...
}
```
This:
- Removes six one-off functions
- Keeps your `handle_prefix_line` callsite untouched
- Forces no extra `String` allocations beyond your existing footnote `format!`
- Keeps the dispatch order obvious and co-located next to the regex table.
</issue_to_address>
### Comment 2
<location> `src/wrap.rs:260` </location>
<code_context>
#[doc(hidden)]
pub fn is_fence(line: &str) -> bool { FENCE_RE.is_match(line) }
+fn is_list_item(line: &str) -> Option<(&str, &str)> {
+ BULLET_RE
+ .captures(line)
</code_context>
<issue_to_address>
The module does not begin with a `//!` comment as required.
Please add a `//!` module-level doc comment at the very top of this file to describe its purpose, as per the review instructions.
</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.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 (1)
src/wrap.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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 / -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.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.
Files:
src/wrap.rs
🔇 Additional comments (5)
src/wrap.rs (5)
7-7: LGTM!The
Capturesimport is properly added to support the function signatures in the newPrefixHandlerstruct.
21-26: LGTM!The
PrefixHandlerstruct effectively encapsulates all the configuration needed for each prefix type. The design is clean and follows single responsibility principle.
34-53: LGTM!The static
HANDLERSarray provides excellent configurability and follows the table-driven approach recommended in previous reviews. The data structure is clear and maintainable.
378-378: LGTM!The labeled loop
'line_loopis appropriately used to enable early continuation from the nested handler iteration.
417-432: LGTM!The table-driven approach excellently consolidates the repetitive prefix handling logic. The implementation correctly iterates over handlers, matches regex patterns, builds prefixes using the configured functions, and continues to the next line after handling. This significantly improves maintainability and reduces code duplication.
1 participant
Summary
wrap_textmain loop focused on high level flowTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_6888f66a0744832287eda00ac83afb5a
Summary by Sourcery
Refactor wrap_text to improve readability and maintainability by extracting block detection and handling logic into dedicated helper functions and simplifying the main loop.
Enhancements: