-
Notifications
You must be signed in to change notification settings - Fork 0
Add renumber restart test #82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,21 +26,61 @@ fn drop_deeper(indent: usize, counters: &mut Vec<(usize, usize)>) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn is_plain_paragraph_line(line: &str) -> bool { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| line.trim_start() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .chars() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .next() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .is_some_and(char::is_alphanumeric) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Remove counters deeper than or equal to `indent`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// ``` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// use mdtablefix::lists::pop_counters_upto; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// let mut counters = vec![(0usize, 1usize), (4, 2), (8, 3)]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// pop_counters_upto(&mut counters, 4); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// assert_eq!(counters, vec![(0, 1)]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// ``` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn pop_counters_upto(counters: &mut Vec<(usize, usize)>, indent: usize) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| while counters.last().is_some_and(|(d, _)| *d >= indent) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| counters.pop(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn handle_paragraph_restart(line: &str, prev_blank: bool, counters: &mut Vec<(usize, usize)>) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let indent_end = line | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .char_indices() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .find(|&(_, c)| !c.is_whitespace()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .map_or_else(|| line.len(), |(i, _)| i); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let indent = indent_len(&line[..indent_end]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if prev_blank | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && counters | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .last() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .is_some_and(|(d, _)| indent <= *d && is_plain_paragraph_line(line)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pop_counters_upto(counters, indent); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+50
to
+64
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Add documentation for the extracted logic. Excellent work extracting the complex paragraph restart logic as suggested in past reviews. Add documentation to explain the function's purpose. +/// Handles resetting list counters when a paragraph line follows a blank line
+/// at or before the indentation level of the last counter.
fn handle_paragraph_restart(line: &str, prev_blank: bool, counters: &mut Vec<(usize, usize)>) {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[must_use] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut prev_blank = lines.first().is_none_or(|l| l.trim().is_empty()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for line in lines { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if is_fence(line) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| in_code = !in_code; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| out.push(line.clone()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| prev_blank = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if in_code { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| out.push(line.clone()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| prev_blank = line.trim().is_empty(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -58,6 +98,7 @@ pub fn renumber_lists(lines: &[String]) -> Vec<String> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| out.push(format!("{indent_str}{current}.{sep}{rest}")); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| prev_blank = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -67,8 +108,10 @@ pub fn renumber_lists(lines: &[String]) -> Vec<String> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .map_or_else(|| line.len(), |(i, _)| i); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let indent_str = &line[..indent_end]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let indent = indent_len(indent_str); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handle_paragraph_restart(line, prev_blank, &mut counters); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| drop_deeper(indent, &mut counters); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| out.push(line.clone()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| prev_blank = line.trim().is_empty(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| out | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| 1. First item | ||
|
|
||
| 2. Second item | ||
|
|
||
| Here’s a plain paragraph within the second item | ||
|
|
||
| 3. Third item | ||
|
|
||
| Here's a plain paragraph at a lower indent level | ||
|
|
||
| 1. Fourth item | ||
|
|
||
| | A | B | | ||
| | --- | --- | | ||
| | 1 | 2 | | ||
|
|
||
| 2. Fifth item | ||
|
|
||
| 3. Sixth item | ||
|
|
||
| ```js | ||
| // fenced code block at lower indent | ||
| console.log('hello'); | ||
| ``` | ||
|
|
||
| 4. Seventh item |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| 1. First item | ||
|
|
||
| 2. Second item | ||
|
|
||
| Here’s a plain paragraph within the second item | ||
|
|
||
| 3. Third item | ||
|
|
||
| Here's a plain paragraph at a lower indent level | ||
|
|
||
| 4. Fourth item | ||
|
|
||
| | A | B | | ||
| | --- | --- | | ||
| | 1 | 2 | | ||
|
|
||
| 5. Fifth item | ||
|
|
||
| 6. Sixth item | ||
|
|
||
| ```js | ||
| // fenced code block at lower indent | ||
| console.log('hello'); | ||
| ``` | ||
|
|
||
| 7. Seventh item |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| use mdtablefix::{lists::pop_counters_upto, renumber_lists}; | ||
|
|
||
| macro_rules! lines_vec { | ||
| ($($line:expr),* $(,)?) => { | ||
| vec![$($line.to_string()),*] | ||
| }; | ||
| } | ||
|
Comment on lines
+3
to
+7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Extract the macro into a test utility module. The Apply this refactor: -macro_rules! lines_vec {
- ($($line:expr),* $(,)?) => {
- vec![$($line.to_string()),*]
- };
-}Create #[macro_export]
macro_rules! lines_vec {
($($line:expr),* $(,)?) => {
vec![$($line.to_string()),*]
};
}Then import it: +mod common;
use mdtablefix::{lists::pop_counters_upto, renumber_lists};
+use common::lines_vec;🤖 Prompt for AI Agents |
||
|
|
||
| #[test] | ||
| fn pop_counters_removes_deeper_levels() { | ||
| let mut counters = vec![(0usize, 1usize), (4, 2), (8, 3)]; | ||
| pop_counters_upto(&mut counters, 4); | ||
| assert_eq!(counters, vec![(0, 1)]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pop_counters_no_change_when_indent_deeper() { | ||
| let mut counters = vec![(0usize, 1usize), (4, 2)]; | ||
| pop_counters_upto(&mut counters, 6); | ||
| assert_eq!(counters, vec![(0, 1), (4, 2)]); | ||
| } | ||
|
Comment on lines
+9
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add edge case tests for pop_counters_upto. The unit tests cover basic scenarios but miss important edge cases that could reveal bugs. Add these additional test cases: #[test]
fn pop_counters_empty_vec() {
let mut counters = vec![];
pop_counters_upto(&mut counters, 0);
assert_eq!(counters, vec![]);
}
#[test]
fn pop_counters_exact_match() {
let mut counters = vec![(0usize, 1usize), (4, 2)];
pop_counters_upto(&mut counters, 4);
assert_eq!(counters, vec![(0, 1)]);
}🤖 Prompt for AI Agents |
||
|
|
||
| #[test] | ||
| fn restart_after_lower_paragraph() { | ||
| let input = lines_vec!("1. One", "", "Paragraph", "3. Next"); | ||
| let expected = lines_vec!("1. One", "", "Paragraph", "1. Next"); | ||
| assert_eq!(renumber_lists(&input), expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_restart_without_blank() { | ||
| let input = lines_vec!("1. One", "Paragraph", "3. Next"); | ||
| let expected = lines_vec!("1. One", "Paragraph", "2. Next"); | ||
| assert_eq!(renumber_lists(&input), expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_restart_for_indented_paragraph() { | ||
| let input = lines_vec!("1. One", "", " Indented", "3. Next"); | ||
| let expected = lines_vec!("1. One", "", " Indented", "2. Next"); | ||
| assert_eq!(renumber_lists(&input), expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn no_restart_for_non_plain_line() { | ||
| let input = lines_vec!("1. One", "", "# Heading", "3. Next"); | ||
| let expected = lines_vec!("1. One", "", "# Heading", "2. Next"); | ||
| assert_eq!(renumber_lists(&input), expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn restart_after_nested_paragraph() { | ||
| let input = lines_vec!("1. One", " 1. Sub", "", "Paragraph", "3. Next"); | ||
| let expected = lines_vec!("1. One", " 1. Sub", "", "Paragraph", "1. Next"); | ||
| assert_eq!(renumber_lists(&input), expected); | ||
| } | ||
|
Comment on lines
+23
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use rstest parameterised tests to reduce duplication. The integration tests follow a repetitive pattern that violates DRY principles. Replace with parameterised tests as mandated by the coding guidelines. Apply this refactor: use rstest::rstest;
#[rstest]
#[case(
lines_vec!("1. One", "", "Paragraph", "3. Next"),
lines_vec!("1. One", "", "Paragraph", "1. Next"),
"restart_after_lower_paragraph"
)]
#[case(
lines_vec!("1. One", "Paragraph", "3. Next"),
lines_vec!("1. One", "Paragraph", "2. Next"),
"no_restart_without_blank"
)]
#[case(
lines_vec!("1. One", "", " Indented", "3. Next"),
lines_vec!("1. One", "", " Indented", "2. Next"),
"no_restart_for_indented_paragraph"
)]
#[case(
lines_vec!("1. One", "", "# Heading", "3. Next"),
lines_vec!("1. One", "", "# Heading", "2. Next"),
"no_restart_for_non_plain_line"
)]
#[case(
lines_vec!("1. One", " 1. Sub", "", "Paragraph", "3. Next"),
lines_vec!("1. One", " 1. Sub", "", "Paragraph", "1. Next"),
"restart_after_nested_paragraph"
)]
fn test_renumber_lists_scenarios(
#[case] input: Vec<String>,
#[case] expected: Vec<String>,
#[case] _description: &str,
) {
assert_eq!(renumber_lists(&input), expected);
}🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Add documentation for the helper function.
Add a doc comment explaining the function's purpose and behaviour.
📝 Committable suggestion
🤖 Prompt for AI Agents