Generalise markdownlint directive detection#145
Merged
Conversation
Contributor
Reviewer's GuideImplements generic detection of markdownlint directives by introducing a regex-based helper, integrating it into text wrapping logic to preserve directive lines, and organizing related tests into a dedicated module. Class diagram for new markdownlint directive detection helperclassDiagram
class wrap {
+is_markdownlint_directive(line: &str) bool
}
class MARKDOWNLINT_DIRECTIVE_RE {
<<static Regex>>
}
wrap --> MARKDOWNLINT_DIRECTIVE_RE : uses
Flow diagram for text wrapping with markdownlint directive detectionflowchart TD
A[Start wrap_text] --> B{Is line a code fence?}
B -- Yes --> C[Flush paragraph, push line, continue]
B -- No --> D{Is line a markdownlint directive?}
D -- Yes --> E[Flush paragraph, push line, continue]
D -- No --> F{Is line empty?}
F -- Yes --> G[Flush paragraph, continue]
F -- No --> H[Buffer line for wrapping]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary by CodeRabbit
WalkthroughIntroduce detection and preservation of markdownlint directive comment lines within the text wrapping logic. Add a regex-based function to identify these directives, ensuring they are output verbatim and break paragraphs. Expand test coverage to verify correct handling of various directive forms and confirm regular comments are still wrapped as usual. Changes
Sequence Diagram(s)sequenceDiagram
participant InputLine as Input Line
participant WrapText as wrap_text()
participant Output as Output
InputLine->>WrapText: Provide next line
WrapText->>WrapText: Check if line is markdownlint directive
alt Is directive
WrapText->>Output: Flush buffer, output directive as-is
WrapText->>WrapText: Clear buffer and indent
else Not a directive
WrapText->>WrapText: Continue normal wrapping logic
end
WrapText->>Output: Output wrapped lines
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 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}
Contributor
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- Consider limiting the visibility of is_markdownlint_directive to pub(crate) (or making it private) if it’s only used internally to avoid exposing it in the public API.
- You can drop the trim() in is_markdownlint_directive since the regex already allows for leading/trailing whitespace, which also lets you preserve the original indentation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider limiting the visibility of is_markdownlint_directive to pub(crate) (or making it private) if it’s only used internally to avoid exposing it in the public API.
- You can drop the trim() in is_markdownlint_directive since the regex already allows for leading/trailing whitespace, which also lets you preserve the original indentation.
## Individual Comments
### Comment 1
<location> `src/wrap.rs:29` </location>
<code_context>
+/// - `<!-- markdownlint-enable -->`
+/// - `<!-- markdownlint-disable-line MD001 MD005 -->`
+/// - `<!-- markdownlint-disable-next-line MD001 -->`
+static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| {
+ Regex::new(
+ r"(?i)^\s*<!--\s*markdownlint-(?:disable|enable|disable-line|disable-next-line)(?:\s+MD\d+)*\s*-->\s*$",
+ )
+ .expect("valid markdownlint regex")
+});
+
/// Markdown token emitted by [`tokenize_markdown`].
</code_context>
<issue_to_address>
The regex for markdownlint directives may not match all valid rule names.
The regex currently only matches rule names like MD\d+, but markdownlint supports custom and plugin rule names (e.g., MD013/line-length). Please update the pattern to support these cases, or document this limitation if it's intentional.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// Matches `markdownlint` comment directives.
///
/// The regex is case-insensitive and supports these forms with optional rule
/// names:
/// - `<!-- markdownlint-disable -->`
/// - `<!-- markdownlint-enable -->`
/// - `<!-- markdownlint-disable-line MD001 MD005 -->`
/// - `<!-- markdownlint-disable-next-line MD001 -->`
static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| {
Regex::new(
r"(?i)^\s*<!--\s*markdownlint-(?:disable|enable|disable-line|disable-next-line)(?:\s+MD\d+)*\s*-->\s*$",
)
.expect("valid markdownlint regex")
});
=======
/// Matches `markdownlint` comment directives.
///
/// The regex is case-insensitive and supports these forms with optional rule
/// names (including custom and plugin rules, e.g. `MD013/line-length`):
/// - `<!-- markdownlint-disable -->`
/// - `<!-- markdownlint-enable -->`
/// - `<!-- markdownlint-disable-line MD001 MD005 MD013/line-length custom-rule -->`
/// - `<!-- markdownlint-disable-next-line MD001 plugin/rule-name -->`
static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| {
Regex::new(
r"(?i)^\s*<!--\s*markdownlint-(?:disable|enable|disable-line|disable-next-line)(?:\s+[A-Za-z0-9_\-/]+)*\s*-->\s*$",
)
.expect("valid markdownlint regex")
});
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tests/markdownlint.rs:63` </location>
<code_context>
+
+/// Comments that resemble directives but are invalid should wrap normally.
+#[test]
+fn test_non_directive_comment_wraps() {
+ let input = lines_vec!["Intro line.", "<!-- markdowndisable -->"];
+ let output = process_stream(&input);
+ assert_eq!(output, lines_vec!["Intro line. <!-- markdowndisable -->"]);
+}
</code_context>
<issue_to_address>
Consider adding tests for malformed or partially correct directives.
Adding tests for nearly correct but invalid directives (e.g., missing closing tag, extra text, or typos) will help verify the regex handles these cases correctly and avoids false matches.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// Comments that resemble directives but are invalid should wrap normally.
#[test]
fn test_non_directive_comment_wraps() {
let input = lines_vec!["Intro line.", "<!-- markdowndisable -->"];
let output = process_stream(&input);
assert_eq!(output, lines_vec!["Intro line. <!-- markdowndisable -->"]);
}
=======
/// Comments that resemble directives but are invalid should wrap normally.
#[test]
fn test_non_directive_comment_wraps() {
let input = lines_vec!["Intro line.", "<!-- markdowndisable -->"];
let output = process_stream(&input);
assert_eq!(output, lines_vec!["Intro line. <!-- markdowndisable -->"]);
}
/// Malformed or partially correct directive comments should wrap normally.
#[test]
fn test_malformed_directive_missing_closing() {
let input = lines_vec!["Text before.", "<!-- markdownlint-disable"];
let output = process_stream(&input);
assert_eq!(output, lines_vec!["Text before. <!-- markdownlint-disable"]);
}
#[test]
fn test_malformed_directive_extra_text() {
let input = lines_vec!["Text before.", "<!-- markdownlint-disable extra -->"];
let output = process_stream(&input);
assert_eq!(output, lines_vec!["Text before. <!-- markdownlint-disable extra -->"]);
}
#[test]
fn test_malformed_directive_typo() {
let input = lines_vec!["Text before.", "<!-- markdownlnt-disable-line MD001 -->"];
let output = process_stream(&input);
assert_eq!(output, lines_vec!["Text before. <!-- markdownlnt-disable-line MD001 -->"]);
}
#[test]
fn test_malformed_directive_incomplete_tag() {
let input = lines_vec!["Text before.", "<!-- markdownlint-disable-line MD001 "];
let output = process_stream(&input);
assert_eq!(output, lines_vec!["Text before. <!-- markdownlint-disable-line MD001 "]);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/markdownlint.rs:15` </location>
<code_context>
+
+/// The disable-next-line directive must remain intact after wrapping.
+#[test]
+fn test_markdownlint_disable_next_line_preserved() {
+ let input = lines_vec and expands on the design ideas described in",
+ "<!-- markdownlint-disable-next-line MD013 -->",
+ ];
+ let output = process_stream(&input);
+ assert_eq!(output, input);
+}
+
</code_context>
<issue_to_address>
Test could include a case where the directive is not at the end of the input.
Please add a test where the markdownlint directive appears in the middle of the input to verify it is preserved in all positions.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
/// The disable-next-line directive must remain intact after wrapping.
#[test]
fn test_markdownlint_disable_next_line_preserved() {
let input = lines_vec and expands on the design ideas described in",
"<!-- markdownlint-disable-next-line MD013 -->",
];
let output = process_stream(&input);
assert_eq!(output, input);
}
=======
/// The disable-next-line directive must remain intact after wrapping.
#[test]
fn test_markdownlint_disable_next_line_preserved() {
let input = lines_vec and expands on the design ideas described in",
"<!-- markdownlint-disable-next-line MD013 -->",
];
let output = process_stream(&input);
assert_eq!(output, input);
}
/// The disable-next-line directive must remain intact when in the middle of the input.
#[test]
fn test_markdownlint_disable_next_line_preserved_middle() {
let input = lines_vec![
"This is the first line.",
"<!-- markdownlint-disable-next-line MD013 -->",
"This is the third line.",
];
let output = process_stream(&input);
assert_eq!(output, input);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/wrap.rs:29` </location>
<code_context>
+/// - `<!-- markdownlint-enable -->`
+/// - `<!-- markdownlint-disable-line MD001 MD005 -->`
+/// - `<!-- markdownlint-disable-next-line MD001 -->`
+static MARKDOWNLINT_DIRECTIVE_RE: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| {
+ Regex::new(
+ r"(?i)^\s*<!--\s*markdownlint-(?:disable|enable|disable-line|disable-next-line)(?:\s+MD\d+)*\s*-->\s*$",
</code_context>
<issue_to_address>
The module does not begin with a `//!` comment as required by the review instructions.
Please add a `//!` module-level comment at the very top of this file to describe its purpose, as per the project guidelines.
</issue_to_address>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
a2sx7j-codex/add-test-for-markdownlint-comment-directives
branch
from
a0b5ee8 to
5d23b5d
Compare
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
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 (2)
src/wrap.rs(3 hunks)tests/markdownlint.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
tests/markdownlint.rssrc/wrap.rs
🧬 Code Graph Analysis (1)
tests/markdownlint.rs (1)
src/process.rs (1)
process_stream(206-214)
🔇 Additional comments (6)
src/wrap.rs (3)
21-35: Excellent regex implementation and documentation.The regex correctly matches markdownlint directives with case-insensitive matching and supports optional rule names including plugin-style names. The documentation clearly explains the supported formats with helpful examples.
325-327: Clean helper function implementation.The function provides a clear API for checking markdownlint directives. The
pub(crate)visibility is appropriate for internal use within the crate.
444-450: Proper integration of markdownlint directive handling.The implementation correctly flushes the paragraph buffer, clears state, and outputs the directive line verbatim. This ensures markdownlint directives remain isolated and unmodified during text wrapping.
tests/markdownlint.rs (3)
1-5: Clear module documentation.The module doc comment properly explains the purpose and follows the required
//!format from the coding guidelines.
60-71: Excellent use of parameterised testing.The
#[rstest]parameterised test efficiently covers multiple directive variants including case variations and indentation. This follows the coding guideline to use rstest for avoiding test duplication.
73-117: Comprehensive malformed directive testing.The tests properly verify that invalid or malformed directives are wrapped normally rather than preserved. This ensures the regex doesn't have false positives and maintains expected wrapping behaviour for non-directive comments.
Comment on lines
+7
to
+11
| use mdtablefix::process_stream; | ||
|
|
||
| #[macro_use] | ||
| mod prelude; | ||
| use prelude::*; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider consolidating imports.
The separate import of mdtablefix::process_stream and the macro use could be organised more cleanly.
Apply this diff to consolidate the imports:
-use mdtablefix::process_stream;
-
-#[macro_use]
-mod prelude;
-use prelude::*;
+#[macro_use]
+mod prelude;
+
+use mdtablefix::process_stream;
+use prelude::*;📝 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.
Suggested change
| use mdtablefix::process_stream; | |
| #[macro_use] | |
| mod prelude; | |
| use prelude::*; | |
| #[macro_use] | |
| mod prelude; | |
| use mdtablefix::process_stream; | |
| use prelude::*; |
🤖 Prompt for AI Agents
In tests/markdownlint.rs around lines 7 to 11, the imports are split between a
macro use declaration and a separate import of mdtablefix::process_stream.
Consolidate these imports by combining the macro use and the process_stream
import into a single organized block at the top of the file, improving clarity
and reducing redundancy.
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.
Summary
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68888eec3afc8322a17eab06cba6d228
Summary by Sourcery
Introduce detection of markdownlint directives in the wrapping logic to prevent them from being wrapped, and add a regex helper along with dedicated tests for directive handling
New Features:
Enhancements:
Tests: