Preserve markdownlint directives when wrapping#130
Conversation
Reviewer's Guidewrap_text now treats markdownlint directives as atomic lines—flushing the current buffer and inserting directives unwrapped—and adds integration tests to verify both directive preservation and regular comment wrapping behavior. Flow diagram for handling markdownlint directives in wrap_textflowchart TD
A[Start line processing] --> B{Is line a markdownlint directive?}
B -- Yes --> C[Flush paragraph buffer]
C --> D[Insert directive line as-is]
D --> E[Continue to next line]
B -- No --> F{Is line empty?}
F -- Yes --> G[Flush paragraph buffer]
G --> H[Continue to next line]
F -- No --> I[Add line to buffer]
I --> E
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughIntroduce a new static regex and helper function to detect markdownlint directive comments within Changes
Sequence Diagram(s)sequenceDiagram
participant InputLine as Input Line
participant WrapText as wrap_text()
participant Output as Output
InputLine->>WrapText: Provide next line
alt Is markdownlint directive
WrapText->>Output: Output line verbatim
WrapText->>WrapText: Flush paragraph buffer
else Is empty line
WrapText->>Output: Output paragraph buffer
else
WrapText->>WrapText: Buffer line for wrapping
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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.
- Extract the markdownlint directive detection into a helper function or regex constant to simplify wrap_text and make it easier to extend in the future.
- Add tests for other markdownlint directive variants (e.g. disable-line, enable) and scenarios with leading indentation to ensure all directive forms are preserved correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the markdownlint directive detection into a helper function or regex constant to simplify wrap_text and make it easier to extend in the future.
- Add tests for other markdownlint directive variants (e.g. disable-line, enable) and scenarios with leading indentation to ensure all directive forms are preserved correctly.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}
leynos
force-pushed
the
i5c327-codex/add-test-for-markdownlint-comment-directives
branch
from
1c290f7 to
021fa03
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/wrap.rs(3 hunks)tests/markdownlint.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/wrap.rstests/markdownlint.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/wrap.rstests/markdownlint.rs
🔇 Additional comments (3)
tests/markdownlint.rs (3)
1-6: LGTM! Excellent module documentation.The module documentation clearly explains the purpose and includes specific examples of the functionality being tested.
24-44: LGTM! Comprehensive test for regular comment wrapping.This test effectively demonstrates that the markdownlint directive detection doesn't interfere with normal comment processing, ensuring the feature is correctly scoped.
13-22: Confirm markdownlint directive preservation.
TheMARKDOWNLINT_DIRECTIVE_REcorrectly matches the trimmed<!-- markdownlint-disable-next-line MD013 -->line, andwrap_textflushes the preceding paragraph then emits the directive unchanged. The test’sassert_eq!(output, input)is valid—no change required.
| /// Matches markdownlint directives including `disable`, `enable`, | ||
| /// `disable-line` and `disable-next-line` forms. | ||
| static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| { | ||
| Regex::new(r"^<!--\s*markdownlint-[^>]*-->$").expect("valid markdownlint regex") | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve regex pattern specificity and documentation.
The current regex ^<!--\s*markdownlint-[^>]*-->$ is too permissive and could match unintended content. It allows any characters after markdownlint- until the closing -->, potentially matching malformed directives.
Replace with a more specific pattern that matches the actual markdownlint directive syntax:
-/// Matches markdownlint directives including `disable`, `enable`,
-/// `disable-line` and `disable-next-line` forms.
-static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| {
- Regex::new(r"^<!--\s*markdownlint-[^>]*-->$").expect("valid markdownlint regex")
-});
+/// Matches markdownlint directives including `disable`, `enable`,
+/// `disable-line` and `disable-next-line` forms with optional rule names.
+static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| {
+ Regex::new(r"^\s*<!--\s*markdownlint-(?:disable|enable|disable-line|disable-next-line)(?:\s+[A-Z0-9\s]+)?\s*-->\s*$")
+ .expect("valid markdownlint regex")
+});📝 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.
| /// Matches markdownlint directives including `disable`, `enable`, | |
| /// `disable-line` and `disable-next-line` forms. | |
| static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| { | |
| Regex::new(r"^<!--\s*markdownlint-[^>]*-->$").expect("valid markdownlint regex") | |
| }); | |
| /// Matches markdownlint directives including `disable`, `enable`, | |
| /// `disable-line` and `disable-next-line` forms with optional rule names. | |
| static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| { | |
| Regex::new(r"^\s*<!--\s*markdownlint-(?:disable|enable|disable-line|disable-next-line)(?:\s+[A-Z0-9\s]+)?\s*-->\s*$") | |
| .expect("valid markdownlint regex") | |
| }); |
🤖 Prompt for AI Agents
In src/wrap.rs around lines 21 to 25, the regex pattern for matching
markdownlint directives is too broad and may match unintended content. Refine
the regex to specifically match valid markdownlint directives such as `disable`,
`enable`, `disable-line`, and `disable-next-line` with optional rule names,
ensuring it does not allow arbitrary characters. Update the regex pattern and
improve the comment to clearly describe the exact directive forms it matches.
| #[doc(hidden)] | ||
| pub fn is_markdownlint_directive(line: &str) -> bool { | ||
| MARKDOWNLINT_DIRECTIVE_RE.is_match(line.trim()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary #[doc(hidden)] attribute.
The #[doc(hidden)] attribute is inappropriate for a function that appears to be part of the module's intended functionality. Based on the test file, this function serves a legitimate purpose in the public API.
Apply this diff to make the function properly documented:
-#[doc(hidden)]
-pub fn is_markdownlint_directive(line: &str) -> bool {
- MARKDOWNLINT_DIRECTIVE_RE.is_match(line.trim())
-}
+/// Check if a line contains a markdownlint directive comment.
+///
+/// Returns `true` for comments matching markdownlint directive patterns
+/// such as `<!-- markdownlint-disable-next-line -->`.
+pub fn is_markdownlint_directive(line: &str) -> bool {
+ MARKDOWNLINT_DIRECTIVE_RE.is_match(line.trim())
+}📝 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.
| #[doc(hidden)] | |
| pub fn is_markdownlint_directive(line: &str) -> bool { | |
| MARKDOWNLINT_DIRECTIVE_RE.is_match(line.trim()) | |
| } | |
| /// Check if a line contains a markdownlint directive comment. | |
| /// | |
| /// Returns `true` for comments matching markdownlint directive patterns | |
| /// such as `<!-- markdownlint-disable-next-line -->`. | |
| pub fn is_markdownlint_directive(line: &str) -> bool { | |
| MARKDOWNLINT_DIRECTIVE_RE.is_match(line.trim()) | |
| } |
🤖 Prompt for AI Agents
In src/wrap.rs around lines 302 to 305, remove the #[doc(hidden)] attribute from
the is_markdownlint_directive function to ensure it is properly documented and
visible as part of the module's public API.
| if is_markdownlint_directive(line) { | ||
| flush_paragraph(&mut out, &buf, &indent, width); | ||
| buf.clear(); | ||
| indent.clear(); | ||
| out.push(line.clone()); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consolidate markdownlint directive handling with existing special case logic.
The markdownlint directive handling duplicates code patterns already present for other special cases (fences, tables, headers). This creates maintenance burden and inconsistency.
Move the markdownlint directive check earlier in the function to group it with other special cases:
}
+ if is_markdownlint_directive(line) {
+ flush_paragraph(&mut out, &buf, &indent, width);
+ buf.clear();
+ indent.clear();
+ out.push(line.clone());
+ continue;
+ }
+
if line.trim_start().starts_with('#') {
flush_paragraph(&mut out, &buf, &indent, width);
buf.clear();
indent.clear();
out.push(line.clone());
continue;
}
- if is_markdownlint_directive(line) {
- flush_paragraph(&mut out, &buf, &indent, width);
- buf.clear();
- indent.clear();
- out.push(line.clone());
- continue;
- }
-
let trimmed = line.trim();🤖 Prompt for AI Agents
In src/wrap.rs around lines 422 to 428, the handling of markdownlint directives
duplicates code already used for other special cases like fences, tables, and
headers. To fix this, move the markdownlint directive check earlier in the
function so it is grouped with the other special case checks. This consolidation
will reduce code duplication and improve maintainability by handling all special
cases in a single, consistent block.
| let trimmed = line.trim(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove redundant variable assignment.
The trimmed variable is assigned but only used once immediately after, creating unnecessary complexity.
Inline the trim() call:
- let trimmed = line.trim();
-
- if trimmed.is_empty() {
+ if line.trim().is_empty() {📝 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.
| let trimmed = line.trim(); | |
| if line.trim().is_empty() { |
🤖 Prompt for AI Agents
In src/wrap.rs around lines 430 to 431, the variable `trimmed` is assigned the
result of `line.trim()` but is only used once immediately after. Remove the
`trimmed` variable and inline the `line.trim()` call directly where `trimmed` is
used to simplify the code.
| /// Other markdownlint directives should also remain on their own lines, even | ||
| /// when indented or combined with multiple rule names. | ||
| #[rstest] | ||
| #[case("<!-- markdownlint-disable-line MD001 MD005 -->")] | ||
| #[case("<!-- markdownlint-enable MD001 -->")] | ||
| #[case(" <!-- markdownlint-disable -->")] | ||
| fn test_markdownlint_directive_variants_preserved(#[case] directive: &str) { | ||
| let input = lines_vec!["A preceding line.", directive]; | ||
| let output = process_stream(&input); | ||
| assert_eq!(output, input); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with edge cases.
The parameterised test covers good basic scenarios but misses important edge cases that could reveal regex pattern issues.
Add additional test cases to cover edge scenarios:
#[rstest]
#[case("<!-- markdownlint-disable-line MD001 MD005 -->")]
#[case("<!-- markdownlint-enable MD001 -->")]
#[case(" <!-- markdownlint-disable -->")]
+#[case("<!-- markdownlint-disable-next-line MD013 MD014 MD015 -->")]
+#[case("<!--markdownlint-disable-->")] // No spaces
+#[case("<!-- markdownlint-enable -->")] // No rule names
+#[case(" <!-- markdownlint-disable-line MD001 --> ")] // Trailing whitespace
fn test_markdownlint_directive_variants_preserved(#[case] directive: &str) {Also add a negative test case to ensure non-markdownlint comments are not preserved:
#[test]
fn test_non_markdownlint_directives_wrap_normally() {
let input = lines_vec![
"Text before comment.",
"<!-- markdown-disable-next-line -->", // Missing 'lint'
"<!-- markdownlint-invalid-directive -->", // Invalid directive
];
let output = process_stream(&input);
// Should wrap these comments, not preserve them
assert_ne!(output, input);
}🤖 Prompt for AI Agents
In tests/markdownlint.rs around lines 46 to 56, the existing parameterized test
covers basic markdownlint directive scenarios but lacks edge cases and negative
tests. Add more test cases to the parameterized test to include edge cases such
as directives with unusual spacing, mixed case, or combined rules. Additionally,
implement a new test function that verifies non-markdownlint comments (e.g.,
comments missing 'lint' or with invalid directives) are not preserved and are
wrapped normally, ensuring the regex pattern correctly distinguishes valid
directives.
|
Superseded by #145 |
1 participant
Summary
markdownlintdirectives on a single lineTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68888eec3afc8322a17eab06cba6d228
Summary by Sourcery
Improve text wrapping to keep markdownlint directives on their own line and add tests to verify this behavior and normal comment wrapping
Enhancements:
Tests: