Conversation
Reviewer's GuideRefactor and simplify Markdown list renumbering by extracting parsing and indentation helpers, streamlining counter stack logic, enhancing documentation, updating README formatting and instructions, and adding targeted tests. Class diagram for refactored list renumbering helpersclassDiagram
class parse_numbered {
<<function>>
+parse_numbered(line: &str) Option<(indent: &str, sep: &str, rest: &str)>
}
class indent_len {
<<function>>
+indent_len(indent: &str) usize
}
class drop_deeper {
<<function>>
+drop_deeper(indent: usize, counters: &mut Vec<(usize, usize)>)
}
class renumber_lists {
+renumber_lists(lines: &[String]) Vec<String>
}
renumber_lists --> parse_numbered : uses
renumber_lists --> indent_len : uses
renumber_lists --> drop_deeper : uses
Class diagram for updated renumber_lists logicclassDiagram
class renumber_lists {
+renumber_lists(lines: &[String]) Vec<String>
-counters: Vec<(usize, usize)>
-out: Vec<String>
}
class parse_numbered {
<<function>>
+parse_numbered(line: &str) Option<(indent: &str, sep: &str, rest: &str)>
}
class indent_len {
<<function>>
+indent_len(indent: &str) usize
}
class drop_deeper {
<<function>>
+drop_deeper(indent: usize, counters: &mut Vec<(usize, usize)>)
}
renumber_lists --> parse_numbered : uses
renumber_lists --> indent_len : uses
renumber_lists --> drop_deeper : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughRefactor the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant renumber_lists
participant parse_numbered
Caller->>renumber_lists: Provide lines (Vec<String>)
loop For each line
renumber_lists->>parse_numbered: Parse indentation, separator, remainder
alt Line is numbered
renumber_lists->>renumber_lists: Update counters stack
renumber_lists->>renumber_lists: Format renumbered line
else Line is not numbered
renumber_lists->>renumber_lists: Output line unchanged
end
end
renumber_lists-->>Caller: Return renumbered lines
Possibly related issues
Possibly related PRs
Poem
✨ 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 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> `src/lib.rs:615` </location>
<code_context>
+
let mut out = Vec::with_capacity(lines.len());
- let mut counters: Vec<(usize, usize)> = Vec::new();
+ let mut stack = Vec::<usize>::new();
+ let mut counters = HashMap::<usize, usize>::new();
let mut in_code = false;
</code_context>
<issue_to_address>
Consider reverting to a single Vec<(indent, count)> for tracking list numbering instead of using both a stack and a HashMap.
Here’s a way to keep the exact same behavior but collapse your two‐structure approach back into a single Vec<(indent, count)>, and even keep using your new `parse_numbered` helper if you like. This is essentially the old “counters” Vec with inline push/pop logic, but using your regex parser:
```rust
pub fn renumber_lists(lines: &[String]) -> Vec<String> {
let mut out = Vec::with_capacity(lines.len());
let mut counters: Vec<(usize, usize)> = Vec::new();
let mut in_code = false;
for line in lines {
if FENCE_RE.is_match(line) {
in_code = !in_code;
out.push(line.clone());
continue;
}
if in_code {
out.push(line.clone());
continue;
}
// Use your parse_numbered helper
if let Some((indent, sep, rest)) = parse_numbered(line) {
// pop any deeper levels
while counters.last().map_or(false, |&(d, _)| d > indent) {
counters.pop();
}
match counters.last_mut() {
Some((d, cnt)) if *d == indent => {
*cnt += 1;
}
_ => {
// new nesting level
counters.push((indent, 1));
}
}
let current = counters.last().unwrap().1;
out.push(format!(
"{}{}.{}{}",
" ".repeat(indent),
current,
sep,
rest
));
continue;
}
// Reset deeper counters when we hit a non‐numbered line
let indent = line.chars().take_while(|c| c.is_whitespace()).count();
while counters.last().map_or(false, |&(d, _)| d > indent) {
counters.pop();
}
out.push(line.clone());
}
out
}
```
Steps to apply:
1. Remove the separate `stack` Vec and the `HashMap`.
2. Replace them with a single `Vec<(usize, usize)> counters` as above.
3. Keep using `parse_numbered` (or inline your regex) for clarity.
4. Pop off deeper levels before deciding to bump or push a new counter.
5. This restores the original one‐structure flow but preserves all new tests and fence‐handling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| let mut out = Vec::with_capacity(lines.len()); | ||
| let mut counters: Vec<(usize, usize)> = Vec::new(); | ||
| let mut stack = Vec::<usize>::new(); |
There was a problem hiding this comment.
issue (complexity): Consider reverting to a single Vec<(indent, count)> for tracking list numbering instead of using both a stack and a HashMap.
Here’s a way to keep the exact same behavior but collapse your two‐structure approach back into a single Vec<(indent, count)>, and even keep using your new parse_numbered helper if you like. This is essentially the old “counters” Vec with inline push/pop logic, but using your regex parser:
pub fn renumber_lists(lines: &[String]) -> Vec<String> {
let mut out = Vec::with_capacity(lines.len());
let mut counters: Vec<(usize, usize)> = Vec::new();
let mut in_code = false;
for line in lines {
if FENCE_RE.is_match(line) {
in_code = !in_code;
out.push(line.clone());
continue;
}
if in_code {
out.push(line.clone());
continue;
}
// Use your parse_numbered helper
if let Some((indent, sep, rest)) = parse_numbered(line) {
// pop any deeper levels
while counters.last().map_or(false, |&(d, _)| d > indent) {
counters.pop();
}
match counters.last_mut() {
Some((d, cnt)) if *d == indent => {
*cnt += 1;
}
_ => {
// new nesting level
counters.push((indent, 1));
}
}
let current = counters.last().unwrap().1;
out.push(format!(
"{}{}.{}{}",
" ".repeat(indent),
current,
sep,
rest
));
continue;
}
// Reset deeper counters when we hit a non‐numbered line
let indent = line.chars().take_while(|c| c.is_whitespace()).count();
while counters.last().map_or(false, |&(d, _)| d > indent) {
counters.pop();
}
out.push(line.clone());
}
out
}Steps to apply:
- Remove the separate
stackVec and theHashMap. - Replace them with a single
Vec<(usize, usize)> countersas above. - Keep using
parse_numbered(or inline your regex) for clarity. - Pop off deeper levels before deciding to bump or push a new counter.
- This restores the original one‐structure flow but preserves all new tests and fence‐handling.
|
@coderabbitai resume |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
✅ Actions performedReviews resumed. |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- parse_numbered could return the indent string slice instead of length to reuse the original whitespace prefix and avoid repeated allocations from " ".repeat.
- There's duplicated logic popping counters in both branches—consider extracting that into a helper to reduce repetition and improve clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- parse_numbered could return the indent string slice instead of length to reuse the original whitespace prefix and avoid repeated allocations from " ".repeat.
- There's duplicated logic popping counters in both branches—consider extracting that into a helper to reduce repetition and improve clarity.
## Individual Comments
### Comment 1
<location> `src/lib.rs:667` </location>
<code_context>
- let indent_len = line.chars().take_while(|c| c.is_whitespace()).count();
- while counters.last().is_some_and(|(i, _)| *i > indent_len) {
+ let indent = line.chars().take_while(|c| c.is_whitespace()).count();
+ while counters.last().is_some_and(|(d, _)| *d > indent) {
counters.pop();
</code_context>
<issue_to_address>
Whitespace counting may not match tab/space semantics in all Markdown renderers.
Some Markdown renderers handle tabs and spaces differently. For full compatibility, consider normalizing tabs to spaces or clearly documenting the current approach.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let indent = line.chars().take_while(|c| c.is_whitespace()).count();
while counters.last().is_some_and(|(d, _)| *d > indent) {
counters.pop();
=======
// Normalize tabs to 4 spaces for consistent indentation counting
let normalized_line = line.replace('\t', " ");
let indent = normalized_line.chars().take_while(|c| c.is_whitespace()).count();
while counters.last().is_some_and(|(d, _)| *d > indent) {
counters.pop();
>>>>>>> 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 indent = line.chars().take_while(|c| c.is_whitespace()).count(); | ||
| while counters.last().is_some_and(|(d, _)| *d > indent) { | ||
| counters.pop(); |
There was a problem hiding this comment.
suggestion: Whitespace counting may not match tab/space semantics in all Markdown renderers.
Some Markdown renderers handle tabs and spaces differently. For full compatibility, consider normalizing tabs to spaces or clearly documenting the current approach.
| let indent = line.chars().take_while(|c| c.is_whitespace()).count(); | |
| while counters.last().is_some_and(|(d, _)| *d > indent) { | |
| counters.pop(); | |
| // Normalize tabs to 4 spaces for consistent indentation counting | |
| let normalized_line = line.replace('\t', " "); | |
| let indent = normalized_line.chars().take_while(|c| c.is_whitespace()).count(); | |
| while counters.last().is_some_and(|(d, _)| *d > indent) { | |
| counters.pop(); |
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 (3)
README.md(4 hunks)src/lib.rs(3 hunks)tests/integration.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🧠 Learnings (1)
README.md (1)
Learnt from: CR
PR: leynos/mdtablefix#0
File: docs/html-table-support.md:0-0
Timestamp: 2025-07-13T13:00:34.632Z
Learning: Applies to docs/**/*.md : `mdtablefix` can format simple HTML `<table>` elements embedded in Markdown by transforming them into Markdown tables before reflow, using the `convert_html_tables` function.
🧬 Code Graph Analysis (1)
tests/integration.rs (1)
src/lib.rs (1)
renumber_lists(647-688)
🪛 LanguageTool
README.md
[style] ~82-~82: Would you like to use the Oxford spelling “recognizes”? The spelling ‘recognises’ is also correct.
Context: ...k. ## HTML table support mdtablefix recognises simple <table> elements embedded in M...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~83-~83: Possible missing comma found.
Context: ... Markdown. Before the main table reflow runs these HTML tables are converted to Mark...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~87-~87: To elevate your writing, try using an alternative expression here.
Context: ... detected, and attributes or tag casing do not matter. After conversion the regular reflow lo...
(MATTERS_RELEVANT)
[uncategorized] ~87-~87: Possible missing comma found.
Context: ...utes or tag casing do not matter. After conversion the regular reflow logic aligns them al...
(AI_HYDRA_LEO_MISSING_COMMA)
[locale-violation] ~98-~98: license must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ... This project is licensed under the ISC license. See the LICENSE file for de...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~98-~98: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...icensed under the ISC license. See the LICENSE file for details.
(LICENCE_LICENSE_NOUN_SINGULAR)
🔇 Additional comments (8)
src/lib.rs (5)
229-240: Well-implemented parsing helper.The
parse_numberedfunction correctly extracts components from numbered list lines using regex capture groups. The explicit extraction of indent, separator, and remainder provides clear separation of concerns.
242-248: Correct tab-to-space conversion logic.The
indent_lenfunction properly handles tab characters by treating them as four spaces, which aligns with the documented behaviour and resolves the whitespace counting concern from past reviews.
250-255: Efficient counter management.The
drop_deeperfunction usesis_some_andfor concise conditional logic and correctly removes counters with deeper indentation levels. This addresses the stack management requirements cleanly.
626-646: Comprehensive documentation with examples.The enhanced documentation includes a clear example and panic note, following Rust documentation best practices. The example demonstrates the expected renumbering behaviour effectively.
664-683: Simplified and robust renumbering logic.The refactored implementation successfully uses the new helper functions to create cleaner, more readable code. The logic correctly handles indentation changes and counter management whilst preserving the original indentation string in the output.
Note: This implementation maintains the single
Vec<(usize, usize)>structure as suggested in past reviews, avoiding the complexity of separate stack and HashMap structures.tests/integration.rs (1)
842-855: Thorough test coverage for tab indentation.The test correctly verifies that nested list items indented with tabs are renumbered properly whilst preserving the tab characters in the output. The test input and expected output clearly demonstrate the intended behaviour of treating tabs as four spaces for indentation calculation.
README.md (2)
31-32: Clear documentation of tab behaviour.The clarification that tabs count as four spaces for indentation in
--renumberaligns perfectly with the implementation changes and addresses potential user confusion about indentation handling.
82-89: Apply British English spelling corrections.The coding guidelines specify en-GB-oxendict spelling. Apply these corrections to align with the project's language standards:
-`mdtablefix` recognises simple `<table>` elements embedded in Markdown. Before -the main table reflow runs these HTML tables are converted to Markdown in a +`mdtablefix` recognises simple `<table>` elements embedded in Markdown. Before +the main table reflow runs, these HTML tables are converted to Markdown in a preprocessing stage handled by `convert_html_tables`. Only basic tables composed of `<tr>`, `<th>` and `<td>` tags are detected, and -attributes or tag casing do not matter. After conversion the regular reflow +attributes or tag casing are irrelevant. After conversion, the regular reflow logic aligns them alongside Markdown tables. See [`docs/html-table-support.md`] (docs/html-table-support.md) for details.⛔ Skipped due to learnings
Learnt from: CR PR: leynos/mdtablefix#0 File: docs/html-table-support.md:0-0 Timestamp: 2025-07-13T13:00:34.632Z Learning: Applies to docs/**/*.md : `mdtablefix` can format simple HTML `<table>` elements embedded in Markdown by transforming them into Markdown tables before reflow, using the `convert_html_tables` function.
| This project is licensed under the ISC license. See the [LICENSE](LICENSE) file | ||
| for details. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Correct licence spelling for British English.
Apply the British English spelling correction:
-This project is licensed under the ISC license. See the [LICENSE](LICENSE) file
+This project is licensed under the ISC licence. See the [LICENCE](LICENCE) file
for details.📝 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.
| This project is licensed under the ISC license. See the [LICENSE](LICENSE) file | |
| for details. | |
| This project is licensed under the ISC licence. See the [LICENCE](LICENCE) file | |
| for details. |
🧰 Tools
🪛 LanguageTool
[locale-violation] ~98-~98: license must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ... This project is licensed under the ISC license. See the LICENSE file for de...
(LICENCE_LICENSE_NOUN_SINGULAR)
[locale-violation] ~98-~98: LICENSE must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...icensed under the ISC license. See the LICENSE file for details.
(LICENCE_LICENSE_NOUN_SINGULAR)
🤖 Prompt for AI Agents
In README.md around lines 98 to 99, the word "license" should be corrected to
the British English spelling "licence". Update the text to use "licence" instead
of "license" to reflect the correct British English spelling.
There was a problem hiding this comment.
"LICENSE" is used here for community consistency
There was a problem hiding this comment.
@leynos, understood. The LICENSE filename is indeed a standard convention in the open source community, and maintaining this consistency makes perfect sense. I'll keep that in mind for future reviews.
✏️ Learnings added
Learnt from: leynos
PR: leynos/mdtablefix#69
File: README.md:98-99
Timestamp: 2025-07-15T07:15:47.096Z
Learning: The LICENSE filename should be kept as-is for community consistency in open source projects, even when using British English spelling elsewhere in documentation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@sourcery-ai review |
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/lib.rs:681` </location>
<code_context>
- while counters.last().is_some_and(|(i, _)| *i > indent_len) {
- counters.pop();
- }
+ let indent_part: String = line.chars().take_while(|c| c.is_whitespace()).collect();
+ let indent = indent_len(&indent_part);
+ drop_deeper(indent, &mut counters);
</code_context>
<issue_to_address>
Consider avoiding allocation by slicing the leading whitespace as a &str instead of collecting it into a String before measuring indentation.
Here’s a quick win: you don’t need to allocate a `String` just to measure the indent on non-numbered lines. Instead, slice off the leading whitespace and pass that `&str` to `indent_len`. This keeps one allocation out of the hot path and makes the non-numbered branch as straightforward as the numbered one.
```rust
// before
let indent_part: String = line.chars().take_while(|c| c.is_whitespace()).collect();
let indent = indent_len(&indent_part);
drop_deeper(indent, &mut counters);
// after
let indent_end = line
.char_indices()
.find(|&(_, c)| !c.is_whitespace())
.map_or(line.len(), |(i, _)| i);
let indent_str = &line[..indent_end];
let indent = indent_len(indent_str);
drop_deeper(indent, &mut counters);
```
This preserves all existing behavior but avoids the intermediate allocation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| while counters.last().is_some_and(|(i, _)| *i > indent_len) { | ||
| counters.pop(); | ||
| } | ||
| let indent_part: String = line.chars().take_while(|c| c.is_whitespace()).collect(); |
There was a problem hiding this comment.
issue (complexity): Consider avoiding allocation by slicing the leading whitespace as a &str instead of collecting it into a String before measuring indentation.
Here’s a quick win: you don’t need to allocate a String just to measure the indent on non-numbered lines. Instead, slice off the leading whitespace and pass that &str to indent_len. This keeps one allocation out of the hot path and makes the non-numbered branch as straightforward as the numbered one.
// before
let indent_part: String = line.chars().take_while(|c| c.is_whitespace()).collect();
let indent = indent_len(&indent_part);
drop_deeper(indent, &mut counters);
// after
let indent_end = line
.char_indices()
.find(|&(_, c)| !c.is_whitespace())
.map_or(line.len(), |(i, _)| i);
let indent_str = &line[..indent_end];
let indent = indent_len(indent_str);
drop_deeper(indent, &mut counters);This preserves all existing behavior but avoids the intermediate allocation.
1 participant
Summary
parse_numberedhelper for numbered line parsingrenumber_listsusing a stack and mapTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_687555d1e5308322a8e4f5878f0cd6cb
Summary by Sourcery
Refactor list renumbering by extracting parsing and indentation helpers, simplify the renumber_lists implementation with a stack-based counter, treat tabs as four spaces for indentation, update documentation, and add a test for tab-indented lists
Enhancements:
Documentation:
Tests: