Conversation
Reviewer's GuideAdds a regex-based Class diagram for updated CLI argument parsing and processingclassDiagram
class Cli {
+bool in_place
+FormatOpts opts
+Vec<PathBuf> files
}
class FormatOpts {
+bool wrap
+bool renumber
+bool breaks
}
Cli --> FormatOpts : has-a
Class diagram for new format_breaks functionclassDiagram
class format_breaks {
+Vec<String> format_breaks(&[String])
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughIntroduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Library
User->>CLI: Run mdtablefix with --breaks
CLI->>CLI: Parse arguments (wrap, renumber, breaks)
CLI->>Library: Call format_breaks(lines) if breaks enabled
Library-->>CLI: Return lines with thematic breaks normalised
CLI-->>User: Output formatted Markdown
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.
- Extract the hardcoded break length (70) into a named constant so it’s easy to adjust and document.
- Consider grouping the wrap/renumber/breaks flags into a single config struct instead of passing multiple booleans through process_lines and rewrite_path.
- Add tests for additional edge cases (e.g. mixed characters, varying leading whitespace) to validate the thematic break regex covers all valid patterns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the hardcoded break length (70) into a named constant so it’s easy to adjust and document.
- Consider grouping the wrap/renumber/breaks flags into a single config struct instead of passing multiple booleans through process_lines and rewrite_path.
- Add tests for additional edge cases (e.g. mixed characters, varying leading whitespace) to validate the thematic break regex covers all valid patterns.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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)src/lib.rs(2 hunks)src/main.rs(3 hunks)tests/integration.rs(2 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
🧬 Code Graph Analysis (1)
tests/integration.rs (2)
src/html.rs (1)
convert_html_tables(294-337)src/lib.rs (2)
format_breaks(549-568)renumber_lists(498-541)
🪛 GitHub Actions: CI
tests/integration.rs
[error] 820-823: Clippy error: calls to push immediately after creation. Consider using the vec![] macro instead of creating a Vec and pushing elements. (clippy::vec_init_then_push)
🔇 Additional comments (12)
src/lib.rs (2)
230-232: LGTM: Regex pattern correctly implements CommonMark thematic break specification.The regex properly matches thematic breaks with up to three leading spaces, followed by three or more matching characters (
*,-, or_) optionally separated by spaces or tabs.
543-568: LGTM: Function implementation follows established patterns and correctly handles fenced code blocks.The
format_breaksfunction properly tracks fenced code block state and only processes thematic breaks outside code blocks. The implementation is consistent with other similar functions in the codebase likerenumber_lists.README.md (2)
23-23: LGTM: Usage line correctly includes the new --breaks option.The command-line usage documentation properly reflects the new formatting option alongside existing flags.
29-29: LGTM: Clear and concise description of the --breaks functionality.The bullet point accurately describes what the --breaks option does, maintaining consistency with other option descriptions.
tests/integration.rs (3)
4-10: LGTM: Import addition is correct and follows existing patterns.The
format_breaksfunction is properly imported alongside other library functions.
827-834: LGTM: Test correctly verifies that code blocks are ignored.The test ensures that thematic breaks inside fenced code blocks remain unchanged, which is the expected behaviour.
836-849: LGTM: CLI integration test properly validates the --breaks option.The test correctly invokes the CLI with the --breaks flag and verifies the expected output format.
src/main.rs (5)
8-8: LGTM: Import addition is correct and necessary.The
format_breaksfunction is properly imported for use in the processing pipeline.
16-33: LGTM: CLI refactoring improves code organisation.The introduction of
FormatOptsstruct effectively groups related formatting flags and makes the CLI structure more maintainable. Thebreaksfield is properly documented and follows existing patterns.
35-48: LGTM: Function signature and implementation correctly handle the new breaks parameter.The
process_linesfunction properly accepts and conditionally applies theformat_breaksfunction when the breaks flag is enabled. The implementation follows the established pattern used for other formatting options.
50-55: LGTM: File rewriting function correctly passes through the breaks parameter.The
rewrite_pathfunction signature and implementation properly thread the breaks parameter toprocess_lines.
87-87: LGTM: All function calls correctly pass the breaks parameter from CLI options.The main function properly extracts the breaks flag from
cli.opts.breaksand passes it to all relevant function calls, ensuring consistent behaviour across different execution paths.Also applies to: 94-94, 98-98
|
Please suggest a fix for the following error and provide a prompt suitable for passing to an agentic AI coding tool: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Here's the fix for the Clippy error and the prompt for an agentic AI coding tool: FixReplace lines 820-823 in let expected = vec![
"foo".to_string(),
"_".repeat(70),
"bar".to_string(),
];This addresses the Agentic AI Coding Tool PromptThis change makes the code more concise, eliminates the need for a mutable variable, and follows Rust best practices. 🏁 Script executed: Length of output: 389 @leynos Perfect! Now I can see the exact context. Here's the precise fix and AI tool prompt: FixReplace lines 820-823 in let expected = vec![
"foo".to_string(),
"_".repeat(70),
"bar".to_string(),
];The complete fixed function will look like: fn test_format_breaks_basic() {
let input = vec!["foo", "***", "bar"]
.into_iter()
.map(str::to_string)
.collect::<Vec<_>>();
let expected = vec![
"foo".to_string(),
"_".repeat(70),
"bar".to_string(),
];
assert_eq!(format_breaks(&input), expected);
}Agentic AI Coding Tool PromptThis change will resolve the Clippy warning and make the code more idiomatic and readable. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)src/lib.rs(2 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
⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
| if !in_code && THEMATIC_BREAK_RE.is_match(line.trim_end()) { | ||
| out.push("_".repeat(THEMATIC_BREAK_LEN)); | ||
| } else { | ||
| out.push(line.clone()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve leading indentation when rewriting thematic breaks
Dropping the original leading whitespace alters the visual alignment of thematic breaks that sit inside nested structures (e.g. list items). Capture the whitespace prefix (up to three spaces per the spec) and prepend it to the generated underscore line.
- out.push("_".repeat(THEMATIC_BREAK_LEN));
+ let indent: String = line.chars().take_while(|c| c.is_whitespace()).collect();
+ out.push(format!("{indent}{}", "_".repeat(THEMATIC_BREAK_LEN)));📝 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.
| if !in_code && THEMATIC_BREAK_RE.is_match(line.trim_end()) { | |
| out.push("_".repeat(THEMATIC_BREAK_LEN)); | |
| } else { | |
| out.push(line.clone()); | |
| } | |
| if !in_code && THEMATIC_BREAK_RE.is_match(line.trim_end()) { | |
| let indent: String = line.chars().take_while(|c| c.is_whitespace()).collect(); | |
| out.push(format!("{indent}{}", "_".repeat(THEMATIC_BREAK_LEN))); | |
| } else { | |
| out.push(line.clone()); | |
| } |
🤖 Prompt for AI Agents
In src/lib.rs around lines 569 to 573, the code replaces thematic break lines
with underscores but drops the original leading whitespace, which breaks
indentation in nested structures. Modify the code to capture up to three leading
spaces from the original line and prepend this whitespace to the generated
underscore line before pushing it to the output, preserving the visual
alignment.
| if !in_code && THEMATIC_BREAK_RE.is_match(line.trim_end()) { | ||
| out.push("_".repeat(THEMATIC_BREAK_LEN)); | ||
| } else { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid repeated heap allocation for the underscore line
"_".repeat(THEMATIC_BREAK_LEN) allocates a fresh String for every thematic break. Store a single static &str and clone it as needed.
+/// Pre-computed 70-underscore break to avoid repeated allocation.
+static THEMATIC_BREAK_LINE: &str = "______________________________________________________________________";
...
- out.push("_".repeat(THEMATIC_BREAK_LEN));
+ let indent: String = line.chars().take_while(|c| c.is_whitespace()).collect();
+ out.push(format!("{indent}{THEMATIC_BREAK_LINE}"));Run cargo +nightly-2025-06-10 fmt --all and cargo clippy -- -D warnings after applying.
📝 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.
| if !in_code && THEMATIC_BREAK_RE.is_match(line.trim_end()) { | |
| out.push("_".repeat(THEMATIC_BREAK_LEN)); | |
| } else { | |
| /// Pre-computed 70-underscore break to avoid repeated allocation. | |
| static THEMATIC_BREAK_LINE: &str = | |
| "______________________________________________________________________"; | |
| // … later in the function … | |
| if !in_code && THEMATIC_BREAK_RE.is_match(line.trim_end()) { | |
| let indent: String = line | |
| .chars() | |
| .take_while(|c| c.is_whitespace()) | |
| .collect(); | |
| out.push(format!("{indent}{THEMATIC_BREAK_LINE}")); | |
| } else { | |
| // … | |
| } |
🤖 Prompt for AI Agents
In src/lib.rs around lines 569 to 571, the code repeatedly allocates a new
String with "_".repeat(THEMATIC_BREAK_LEN) for each thematic break, causing
unnecessary heap allocations. To fix this, define a static &str with the
underscore line repeated THEMATIC_BREAK_LEN times once, then clone or reuse this
static string whenever needed instead of calling repeat each time. After making
this change, run cargo +nightly-2025-06-10 fmt --all and cargo clippy -- -D
warnings to ensure formatting and linting compliance.
| - Use `--breaks` to normalize thematic breaks to a line of 70 underscores | ||
| (configurable via the `THEMATIC_BREAK_LEN` constant). |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use en-GB spelling and finish the bullet with a full stop
Maintain stylistic consistency with the rest of the bullets.
- Use `--breaks` to normalize thematic breaks to a line of 70 underscores
- (configurable via the `THEMATIC_BREAK_LEN` constant).
+ Use `--breaks` to normalise thematic breaks to a line of 70 underscores.
+ (Configurable via the `THEMATIC_BREAK_LEN` constant.)📝 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.
| - Use `--breaks` to normalize thematic breaks to a line of 70 underscores | |
| (configurable via the `THEMATIC_BREAK_LEN` constant). | |
| - Use `--breaks` to normalise thematic breaks to a line of 70 underscores. | |
| (Configurable via the `THEMATIC_BREAK_LEN` constant.) |
🤖 Prompt for AI Agents
In README.md around lines 31 to 32, update the bullet point to use en-GB
spelling by changing "normalize" to "normalise" and ensure the sentence ends
with a full stop to maintain stylistic consistency with other bullet points.
There was a problem hiding this comment.
This repo uses en-oxendict spelling
There was a problem hiding this comment.
@leynos Thank you for the correction! You're absolutely right. I apologise for the error in my previous suggestion. Oxford spelling (en-oxendic) uses "-ize" endings, so "normalize" is indeed the correct spelling for this repository. I should not have suggested changing it to "normalise".
✏️ Learnings added
Learnt from: leynos
PR: leynos/mdtablefix#57
File: README.md:31-32
Timestamp: 2025-07-13T22:20:49.785Z
Learning: This repository uses en-oxendic (Oxford) spelling for markdown files, which means using "-ize" endings (like "normalize") rather than "-ise" endings (like "normalise"), while maintaining other British spelling conventions.
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.
1 participant
Summary
--breaksoption in the CLIformat_breaksto normalise thematic breaksTesting
cargo +nightly-2025-06-10 fmt --allcargo clippy -- -D warningsRUSTFLAGS="-D warnings" cargo testmarkdownlint *.md docs/*.mdhttps://chatgpt.com/codex/tasks/task_e_6873b23756788322b41601109565b38f
Summary by Sourcery
Introduce a --breaks option to the CLI and implement logic to normalize thematic break lines as 70 underscores, integrating it into the processing pipeline and documenting and testing the new feature.
New Features:
Enhancements:
Documentation:
Tests: