Conversation
Reviewer's GuideThis PR refactors table reflow logic to compute and apply uniform column widths, exposes internal helpers for doctests, updates examples accordingly, and adds integration tests to verify uniform padding and non-table line preservation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe updates introduce improved Markdown table formatting by refactoring the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Library
User->>Library: Call reflow_table(input)
Library->>Library: Split rows into cells
Library->>Library: Calculate max column widths
Library->>Library: Pad each cell to column width
Library->>User: Return reflowed table string
Possibly related PRs
Poem
🪧 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:
- The two-phase approach in reflow_table (first building
cleaned, then computingwidths) could be merged into a single pass to improve readability and avoid scanning the data twice. - Dropping all empty cell strings with
row.retain(|c| !c.is_empty())will remove intentionally blank columns—consider preserving empty strings and only padding missing cells instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The two-phase approach in reflow_table (first building `cleaned`, then computing `widths`) could be merged into a single pass to improve readability and avoid scanning the data twice.
- Dropping all empty cell strings with `row.retain(|c| !c.is_empty())` will remove intentionally blank columns—consider preserving empty strings and only padding missing cells instead.
## Individual Comments
### Comment 1
<location> `tests/integration.rs:209` </location>
<code_context>
+ "* bullet".to_string(),
+ String::new(),
+ ];
+ assert_eq!(output, expected);
+}
</code_context>
<issue_to_address>
Add a test case for `process_stream` with input containing no tables.
Add a test where the input contains only non-table Markdown elements to verify that `process_stream` returns the input unchanged.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
Command::cargo_bin("mdtablefix")
.unwrap()
.success()
.stdout("| A | B |\n| 1 | 2 |\n| 3 | 4 |\n");
}
#[test]
fn test_uniform_example_one() {
=======
Command::cargo_bin("mdtablefix")
.unwrap()
.success()
.stdout("| A | B |\n| 1 | 2 |\n| 3 | 4 |\n");
}
#[test]
fn test_process_stream_no_tables() {
let input = vec![
"# Heading".to_string(),
String::new(),
"Some paragraph text.".to_string(),
String::new(),
"* List item 1".to_string(),
"* List item 2".to_string(),
String::new(),
"> Blockquote".to_string(),
String::new(),
"1. Numbered list".to_string(),
String::new(),
];
let output = process_stream(input.clone());
assert_eq!(output, input);
}
#[test]
fn test_uniform_example_one() {
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
leynos
force-pushed
the
codex/uniformly-pad-table-columns
branch
from
57c78e9 to
98e2209
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/lib.rs (1)
86-107:⚠️ Potential issueEmpty-cell stripping corrupts table structure
row.retain(|c| !c.is_empty())(and the earlierfilter(|c| !c.is_empty())) silently drop legitimately empty cells.
For a row such as| Alice | | 42 |, the empty Age column disappears and the data shift one position to the left, producing an invalid table.- row.retain(|c| !c.is_empty()); - while row.len() < max_cols { - row.push(String::new()); - } + // Keep *all* cells – blank cells are meaningful. + if row.len() < max_cols { + row.extend(std::iter::repeat(String::new()).take(max_cols - row.len())); + }You also need to compute
max_colsfromrow.len()(not the non-empty count) and remove the correspondingfilter(...).count()logic above.Failing to fix this will yield data loss and wrong column alignment when blank cells are present.
🧹 Nitpick comments (4)
src/lib.rs (2)
58-60: Compile the sentinel regex once
Regex::newis executed every timereflow_tableruns, incurring unnecessary recompilation overhead.
Move it into alazy_static!/once_cell::sync::Lazyso it is compiled once and re-used.
108-124: Minor: tailor the separator row for nicer outputThe current padding inserts spaces into the
---alignment row, e.g.| --- |→| --- |.
While valid CommonMark, visually it looks odd.
Consider detecting rows that consist solely of-,:, and spaces and extending them with'-'instead of spaces:if row.iter().all(|c| c.trim_matches(':').chars().all(|ch| ch == '-')) { // rebuild with repeated '-' up to widths[i] }Purely cosmetic, feel free to skip.
README.md (1)
5-5: Tiny wording tweak“A tool for reflowing Markdown tables …” reads a bit smoother than “A tool to reflow …”, but this is purely stylistic.
tests/integration.rs (1)
134-210: Width check includes leading spacesThe uniform-width tests measure
str::len()on the raw split cells, which retain the leading space after the pipe.
Although the test passes today, trimming would make the intent clearer:- assert_eq!(col.len(), widths[i]); + assert_eq!(col.trim().len(), widths[i].saturating_sub(1));Not mandatory, but avoids future confusion if the join delimiter changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/lib.rs(5 hunks)tests/integration.rs(2 hunks)
🔇 Additional comments (1)
tests/integration.rs (1)
124-125: Good use of the new{ident}formatting syntaxSwitching to
writeln!(f, "{line}")is concise and idiomatic. 👍
1 participant
Summary
split_cellsfor doctestsTesting
cargo clippy -- -D warningscargo clippy --tests -- -D warningscargo testhttps://chatgpt.com/codex/tasks/task_e_684c6cef66708322aba3799b34b2a8f8
Summary by Sourcery
Compute and apply uniform column widths when reflowing Markdown tables and ensure non-table lines remain unchanged
New Features:
Bug Fixes:
Enhancements:
Documentation:
no_runannotation where appropriateTests:
Summary by CodeRabbit