Address review feedback for wrapping and footnotes#266
Open
Conversation
Centralize mutable state in the list, HTML-table, footnote, and paragraph-wrapping flows so the behaviour is easier to reason about from one place. Fix the cargo-binstall Linux override, remove production `expect` usage from the reviewed paths, normalize whitespace-only wrapped lines, and add regression tests for mixed code-emphasis affixes, Unicode-width indentation, and indented multi-line HTML tables.
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactor internal ownership for wrapping and HTML-table buffering; extract footnote-definition renumbering into a new submodule; consolidate list-numbering logic; update wrap-related APIs and tests; tweak Cargo bin-dir metadata. Changes
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideRefactors wrapping, list renumbering, footnote renumbering, and HTML table conversion to centralize mutable state and remove panicking expectations, while tightening wrapping behavior (Unicode-aware indentation, whitespace-only lines) and adding regression tests, plus a small cargo-binstall override fix. Sequence diagram for updated wrap_text paragraph processingsequenceDiagram
participant WT as wrap_text
participant FT as FenceTracker
participant PW as ParagraphWriter
loop for each line in lines
WT->>FT: observe(line)
alt fence line toggled
FT-->>WT: true
WT->>PW: push_verbatim(line)
WT->>WT: continue
else not fence
FT-->>WT: false
alt FT.in_fence()
WT->>PW: push_verbatim(line)
WT->>WT: continue
else not in fence
alt line.trim().is_empty()
WT->>PW: push_blank_line()
WT->>WT: continue
else
alt passthrough block
WT->>PW: push_verbatim(line)
WT->>WT: continue
else prefix_line found
WT->>PW: handle_prefix_line(prefix_line)
WT->>WT: continue
else plain paragraph text
WT->>PW: note_indent(line)
WT->>WT: (text, hard_break) = line_break_parts(line)
WT->>PW: push_wrapped(text, hard_break)
end
end
end
end
end
WT->>PW: flush_paragraph()
Class diagram for updated wrapping, list, footnote, and HTML table state structsclassDiagram
class ParagraphWriter {
- out : &mut Vec~String~
- width : usize
- buf : Vec~(String, bool)~
- indent : String
+ new(out : &mut Vec~String~, width : usize) ParagraphWriter
+ note_indent(line : &str) void
+ push_wrapped(text : String, hard_break : bool) void
+ flush_paragraph() void
+ push_verbatim(line : &str) void
+ push_blank_line() void
+ handle_prefix_line(prefix_line : &PrefixLine) void
- push_wrapped_segment(indent : &str, segment : &str) void
}
class PrefixLine {
+ prefix : String
+ rest : &str
+ repeat_prefix : bool
}
class SplitContext {
+ lines : &mut Vec~String~
+ width : usize
+ new(lines : &mut Vec~String~, width : usize) SplitContext
}
class HtmlTableState {
+ buf : Vec~String~
+ depth : usize
+ in_html() bool
+ flush_raw(out : &mut Vec~String~) void
+ push_html_line(line : &str, out : &mut Vec~String~) void
}
class ListState {
+ indent_stack : Vec~usize~
+ counters : HashMap~usize, usize~
+ reset() void
+ prune_deeper(indent : usize, inclusive : bool) void
+ next_number(indent : usize) usize
+ handle_paragraph_restart(indent : usize, line : &str, prev_blank : bool) bool
}
class DefinitionScanState {
+ mapping : &mut HashMap~usize, usize~
+ next_number : &mut usize
+ numeric_list_range : Option~(usize, usize)~
+ skip_numeric_conversion : bool
+ definitions : Vec~DefinitionLine~
+ is_definition_line : Vec~bool~
+ numeric_candidates : Vec~NumericCandidate~
}
class FenceTracker {
+ observe(line : &str) bool
+ in_fence() bool
}
ParagraphWriter --> PrefixLine : handles
ParagraphWriter --> SplitContext : uses via wrap_preserving_code
ParagraphWriter --> FenceTracker : used in wrap_text
HtmlTableState ..> table_lines_to_markdown : calls
ListState ..> FenceTracker : used in renumber_lists
DefinitionScanState ..> DefinitionLine : owns
DefinitionScanState ..> NumericCandidate : owns
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/footnotes/renumber.rs (1)
337-344:⚠️ Potential issue | 🟠 MajorRemove the panic path in
numeric_candidate_from_line.Parse from
num_match.as_str()directly. Line 339 still indexescaps["num"], which panics when the named group is absent and defeats theOption-based fallback.🔧 Proposed fix
fn numeric_candidate_from_line(line: &str, index: usize) -> Option<NumericCandidate> { let caps = FOOTNOTE_LINE_RE.captures(line)?; - let number = caps["num"].parse::<usize>().ok()?; + let num_match = caps.name("num")?; + let number = num_match.as_str().parse::<usize>().ok()?; let indent = caps.name("indent").map_or("", |m| m.as_str()).to_string(); - let rest = caps.name("rest").map_or("", |m| m.as_str()).to_string(); - let num_match = caps.name("num")?; - let rest_match = caps.name("rest")?; + let rest_match = caps.name("rest")?; + let rest = rest_match.as_str().to_string(); let whitespace = line[num_match.end() + 1..rest_match.start()].to_string(); Some(NumericCandidate { index, number, indent,As per coding guidelines: ".expect() and .unwrap() are forbidden outside of tests. Errors must be propagated."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/footnotes/renumber.rs` around lines 337 - 344, The function numeric_candidate_from_line currently uses caps["num"] which can panic if the named capture is absent; instead use the already-obtained num_match (from caps.name("num")) and parse num_match.as_str() to get the usize (e.g., replace the caps["num"].parse::<usize>() call with parsing num_match.as_str()), keeping the .ok()? propagation so the function returns None on parse failure; also remove any other direct indexing into caps[...] that can panic and rely on the existing num_match/rest_match variables and safe slicing using their start()/end() positions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/footnotes/renumber.rs`:
- Around line 328-329: The code currently discards the Result from write! when
building the definition header (e.g., the line using write!(&mut line,
"[^{new_number}]:") before pushing rewritten_rest), creating a silent failure
path; replace those write! calls (both occurrences around the build of the
header) with infallible string assembly such as let header =
format!("[^{new_number}]:"); line.push_str(&header); (or build the header with
String::from and push_str) so you don't ignore errors and avoid using a fallible
fmt write into the mutable string; apply the same replacement for the second
occurrence near lines 401–402.
In `@src/wrap/tests.rs`:
- Around line 309-314: The test wrap_text_uses_display_width_for_unicode_indent
is too weak: it uses " a" with width 2 which doesn't fail even if indent width
is computed incorrectly; update the test to exercise display-width by using an
input like " a b" and call wrap_text(&input, 4) so the ideographic space (width
2) causes a wrap, and assert that the result equals vec![" a".to_string(),
" b".to_string()]—modify the test body in
wrap_text_uses_display_width_for_unicode_indent to use that input, width, and
expected assertion.
- Around line 302-314: The new regression tests
wrap_text_normalizes_whitespace_only_lines and
wrap_text_uses_display_width_for_unicode_indent should be moved out of the
already-large src/wrap/tests.rs into a dedicated test module file to keep files
under the 400-line limit; create a new test file (for example
tests/wrap_regressions.rs or src/wrap/regressions_tests.rs), copy those two
#[test] functions (which call wrap_text) into it, preserve any necessary
use/imports for wrap_text, and remove them from src/wrap/tests.rs so the
original file stays below the line limit.
---
Outside diff comments:
In `@src/footnotes/renumber.rs`:
- Around line 337-344: The function numeric_candidate_from_line currently uses
caps["num"] which can panic if the named capture is absent; instead use the
already-obtained num_match (from caps.name("num")) and parse num_match.as_str()
to get the usize (e.g., replace the caps["num"].parse::<usize>() call with
parsing num_match.as_str()), keeping the .ok()? propagation so the function
returns None on parse failure; also remove any other direct indexing into
caps[...] that can panic and rely on the existing num_match/rest_match variables
and safe slicing using their start()/end() positions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d67b626d-6713-4149-8d79-2492c7423851
📒 Files selected for processing (12)
Cargo.tomlsrc/code_emphasis.rssrc/footnotes/renumber.rssrc/html.rssrc/lists.rssrc/wrap.rssrc/wrap/fence.rssrc/wrap/inline.rssrc/wrap/line_buffer.rssrc/wrap/paragraph.rssrc/wrap/tests.rstests/table/convert_html.rs
Keep HTML table depth tracking on a consistent trimmed view, restore borrowed prefix handling for common wrap prefixes, and add regression coverage for whitespace-only paragraph breaks and indented HTML table transitions. Document why numeric footnote candidates are finalized in reverse and add targeted renumbering tests covering existing definition headers, numeric candidates, and malformed candidate lines.
Replace the silent `write!` header assembly in the footnote renumberer with infallible string construction, and stop parsing numeric candidates through panic-prone capture indexing. Move the `wrap_text` regression coverage out of the oversized `src/wrap/tests.rs` file into the existing dedicated integration test surface, and strengthen the Unicode-indent case so it proves display width rather than byte length.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/wrap/tests.rs (2)
302-324: 🛠️ Refactor suggestion | 🟠 MajorMove these regressions into a dedicated test module.
Lines 302-324 keep
src/wrap/tests.rsat 489 lines, so this PR still breaches the repository cap. Move the new wrapping regressions out of this module and keep the file below the limit.As per coding guidelines, "Files must not exceed 400 lines in length".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wrap/tests.rs` around lines 302 - 324, The three new regression tests (wrap_text_normalizes_whitespace_only_lines, wrap_text_treats_whitespace_only_lines_as_paragraph_breaks, wrap_text_uses_display_width_for_unicode_indent) should be moved out of the oversized tests.rs into a dedicated test module/file (e.g., a new module named wrap_regressions or regressions) so the original file stays under the 400-line limit; extract those #[test] functions into the new module, ensure the new file/module is compiled as part of the test suite (keeping the same function names and signatures and any necessary use/imports such as wrap_text), and remove the original copies from the large tests.rs.
320-324:⚠️ Potential issue | 🟡 MinorStrengthen the Unicode-width regression.
Lines 320-324 still pass when the ideographic space is mismeasured as one column, so the test does not prove the fix. Use an input that only wraps when that indent consumes two columns, such as
" a b"at width4, and assert the split output.Patch
fn wrap_text_uses_display_width_for_unicode_indent() { - let input = vec![" a".to_string()]; - let wrapped = wrap_text(&input, 2); - assert_eq!(wrapped, vec![" a".to_string()]); + let input = vec![" a b".to_string()]; + let wrapped = wrap_text(&input, 4); + assert_eq!(wrapped, vec![" a".to_string(), " b".to_string()]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wrap/tests.rs` around lines 320 - 324, The test wrap_text_uses_display_width_for_unicode_indent currently uses " a" which still passes if the ideographic space is mismeasured; change it to exercise the wrap threshold by using input " a b" and width 4 so the indent (ideographic space = 2 columns) forces a wrap; update the test (function wrap_text_uses_display_width_for_unicode_indent) to call wrap_text(&vec![" a b".to_string()], 4) and assert the expected split output (e.g. assert_eq!(wrapped, vec![" a".to_string(), "b".to_string()])).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/footnotes/renumber.rs`:
- Around line 490-528: This file is over the 400-line limit because the
#[cfg(test)] mod tests block is large; move the entire test module into a new
test file (either src/footnotes/renumber/tests.rs or
tests/footnotes_renumber.rs) preserving the imported functions
numeric_candidate_from_line and renumber_footnotes, and update module imports
accordingly; also replace the duplicated assertions in the
malformed_numeric_candidate_line_is_ignored test with a parameterised rstest
(use #[rstest] with two cases like "7." and "7:") to consolidate into a single
parameterised test function while keeping the existing test names for clarity.
In `@tests/table/convert_html.rs`:
- Around line 81-115: Collapse the two nearly-identical tests into a single
parameterised rstest: remove the functions
converts_indented_multiline_html_table and
converts_indented_table_without_touching_surrounding_content and replace them
with one #[rstest(...)] that supplies the two cases and a single test function
(e.g. converts_indented_html_table_cases) which takes input: Vec<String>,
expected: Vec<String> and asserts assert_eq!(convert_html_tables(&input),
expected); ensure the rstest attribute lists both case::... entries and the test
helper convert_html_tables is used unchanged.
---
Duplicate comments:
In `@src/wrap/tests.rs`:
- Around line 302-324: The three new regression tests
(wrap_text_normalizes_whitespace_only_lines,
wrap_text_treats_whitespace_only_lines_as_paragraph_breaks,
wrap_text_uses_display_width_for_unicode_indent) should be moved out of the
oversized tests.rs into a dedicated test module/file (e.g., a new module named
wrap_regressions or regressions) so the original file stays under the 400-line
limit; extract those #[test] functions into the new module, ensure the new
file/module is compiled as part of the test suite (keeping the same function
names and signatures and any necessary use/imports such as wrap_text), and
remove the original copies from the large tests.rs.
- Around line 320-324: The test wrap_text_uses_display_width_for_unicode_indent
currently uses " a" which still passes if the ideographic space is mismeasured;
change it to exercise the wrap threshold by using input " a b" and width 4 so
the indent (ideographic space = 2 columns) forces a wrap; update the test
(function wrap_text_uses_display_width_for_unicode_indent) to call
wrap_text(&vec![" a b".to_string()], 4) and assert the expected split output
(e.g. assert_eq!(wrapped, vec![" a".to_string(), "b".to_string()])).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e0b8687-eb63-4e39-946c-a8b476a89d3d
📒 Files selected for processing (6)
src/footnotes/renumber.rssrc/html.rssrc/wrap.rssrc/wrap/paragraph.rssrc/wrap/tests.rstests/table/convert_html.rs
Owner
Author
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12108 🏁 Script executed: Length of output: 7747
Overall Comments
Individual Comments
|
Move the footnote renumber unit tests into a dedicated submodule file and extract the dense definition-scanning and reordering helpers into a separate implementation module. This brings `renumber.rs` back under the repository file-size limit without changing the top-level renumbering flow. Collapse the duplicated indented HTML table regression tests into a single `rstest` so the behaviour stays covered with less repetition.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/footnotes/renumber/definitions.rs`:
- Line 100: Replace the odd double-super path by re-exporting or importing
DefinitionParts from the immediate parent: in renumber.rs add a re-export/import
for parsing::DefinitionParts (e.g., make DefinitionParts available as
super::parsing::DefinitionParts) and then update the type in definitions.rs to
use the simpler super::parsing::DefinitionParts<'_> (replace
super::super::parsing::DefinitionParts<'_> occurrences). This keeps the module
path clear and locates the change around the DefinitionParts type usage in
definitions.rs and the renumber.rs parent module.
In `@src/footnotes/renumber/tests.rs`:
- Around line 13-18: The test malformed_numeric_candidate_line_is_ignored uses
assert! without a diagnostic message; update the assertion to include a clear
failure message that shows the input, e.g. change the assertion around
numeric_candidate_from_line(line, 0).is_none() to include a message like
"expected None for malformed numeric candidate line: {line}" so failures surface
the offending case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3caf340c-8cb1-447f-8018-395c17e6be97
📒 Files selected for processing (4)
src/footnotes/renumber.rssrc/footnotes/renumber/definitions.rssrc/footnotes/renumber/tests.rstests/table/convert_html.rs
Expose `DefinitionParts` through the immediate `renumber` parent module so the definition-scanning helper module no longer reaches back through a double-super path. This keeps the module boundary clearer without changing the renumbering behaviour.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/footnotes/renumber.rs (1)
70-87:⚠️ Potential issue | 🟠 MajorReplace
expectwith fallible handling in production code.Line 73 uses
expectin production code, which violates the guideline forbidding.expect()outside tests. Return early when the capture is missing instead of panicking.🔧 Proposed fix
fn rewrite_refs_in_segment(text: &str, mapping: &HashMap<usize, usize>) -> String { FOOTNOTE_REF_RE .replace_all(text, |caps: &Captures| { - let mat = caps.get(0).expect("regex matched without capture"); + let Some(mat) = caps.get(0) else { + return caps[0].to_string(); + }; if is_definition_like(text, &mat) { return caps[0].to_string(); }As per coding guidelines: "
.expect()and.unwrap()are forbidden outside of tests. Errors must be propagated."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/footnotes/renumber.rs` around lines 70 - 87, The closure in rewrite_refs_in_segment currently calls caps.get(0).expect(...), which can panic; replace that with fallible handling: match or if-let on caps.get(0) (e.g., if let Some(mat) = caps.get(0) { ... } else { ... }), and in the None branch return an appropriate safe fallback (the original matched text or String::new()) instead of panicking; keep the rest of the logic (calling is_definition_like(text, &mat), parsing caps["num"], and using mapping) unchanged so the closure never uses .expect() or .unwrap().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/footnotes/renumber.rs`:
- Around line 70-87: The closure in rewrite_refs_in_segment currently calls
caps.get(0).expect(...), which can panic; replace that with fallible handling:
match or if-let on caps.get(0) (e.g., if let Some(mat) = caps.get(0) { ... }
else { ... }), and in the None branch return an appropriate safe fallback (the
original matched text or String::new()) instead of panicking; keep the rest of
the logic (calling is_definition_like(text, &mat), parsing caps["num"], and
using mapping) unchanged so the closure never uses .expect() or .unwrap().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63e834dd-352d-4c55-b0ab-ef615c11d4b0
📒 Files selected for processing (2)
src/footnotes/renumber.rssrc/footnotes/renumber/definitions.rs
Owner
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
Handle the impossible-but-fallible `caps.get(0)` path in `rewrite_refs_in_segment` without panicking. If the regex capture is missing, return a safe empty replacement instead of aborting the renumbering pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Centralize mutable state in the list, HTML-table, footnote, and paragraph-wrapping flows so the behaviour is easier to reason about from one place.
Fix the cargo-binstall Linux override, remove production
expectusage from the reviewed paths, normalize whitespace-only wrapped lines, and add regression tests for mixed code-emphasis affixes, Unicode-width indentation, and indented multi-line HTML tables.Summary by Sourcery
Centralize mutable state across paragraph wrapping, footnote renumbering, list renumbering, and HTML table conversion to simplify control flow and improve robustness.
Bug Fixes:
Enhancements:
Build:
Tests: