feat(#15): markdownlint for check comments
#505
Conversation
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
|
@h1alexbel can you please check this one? |
h1alexbel
left a comment
h1alexbel
left a comment
There was a problem hiding this comment.
@Marat-Tim looks like a good start! Take a look at my comments, please
pom.xml
Outdated
| </configuration> | ||
| </execution> | ||
| <execution> | ||
| <id>copy-js</id> |
There was a problem hiding this comment.
@h1alexbel does this apply only to the maven id, or do I need to rename all js uses to javascript (for example, in folder names and comments)?
| * This lint works for multiple files, as creating a MarkdownLinter takes a very long time | ||
| * and creating it many times is a bad idea. | ||
| * | ||
| * @since 0.57.0 |
| */ | ||
| private static final Set<String> IGNORED = Set.of( | ||
| "MD041", | ||
| "MD047", |
There was a problem hiding this comment.
@Marat-Tim can we enable MD047? It seems it does good service: https://github.com/DavidAnson/markdownlint/blob/main/doc/md047.md
There was a problem hiding this comment.
@h1alexbel do we want comments on objects to have an empty line at the end?
It will look like this
# Main object.
#
[] > foo
Is this exactly what we want? That's not how they write it in the EO code I've seen.
There was a problem hiding this comment.
@Marat-Tim indeed, you are right. We should ignore this rule
|
|
||
| @Override | ||
| public String name() { | ||
| return "lt-markdown-only"; |
There was a problem hiding this comment.
@Marat-Tim let's simplify the name to invalid-markdown-comment. Java class can be renamed to LtInvalidMarkdownComment
| * | ||
| * @since 0.57.0 | ||
| */ | ||
| final class LtMarkdownOnly implements Lint<Map<String, XML>> { |
There was a problem hiding this comment.
@Marat-Tim Do we need to analyze multiple programs in the scope to issue the defects? I think that single XMIR scope should be enough for this lint. So, we should implement Lint<XML> instead. Or I'm missing something?
There was a problem hiding this comment.
@h1alexbel I did this because it takes a lot of time to create a MarkdownLinter and it is very expensive to create it for each file. It is better to reuse it. We can require an instance of it in the class constructor and shift responsibility for its lifetime to the library user, but in my opinion this complicates our API
| * | ||
| * @since 0.57.0 | ||
| */ | ||
| public interface MarkdownDefect { |
There was a problem hiding this comment.
@Marat-Tim why new class here? Maybe we can use Defect.Simple?
There was a problem hiding this comment.
@h1alexbel I think not. Ideologically, Defect is an error in the eolang. But this class is an interop between the java class and the javascript class. I don't think it's worth mixing them up
There was a problem hiding this comment.
@Marat-Tim I think we just transferring them from markdownlint to the lints. Thats why its reasonable to use Defect.Simple and not overcomplicate the logic with new class
There was a problem hiding this comment.
@h1alexbel no, this error appears due to the analysis of plain text (not XMIR). It has no severity, it has no name, it has no version and context. This is a completely different logical entity
There was a problem hiding this comment.
@Marat-Tim yes, but MarkdownDefect is used only as intermediate class. Eventually, in the lint you map MarkdownDefect into Defect.Simple. Seems to be redundant to me. Can we directly produce Defect.Simple without mapping?
There was a problem hiding this comment.
@h1alexbel Are you suggesting that the MarkdownLinter class returns a Stream<Defect.Default>? Then what should I specify in values that are not present in the context of markdownlint (such as severity, version, context, and others)? Should I specify null there?
There was a problem hiding this comment.
@Marat-Tim severity is the same you specify here (WARNING):
defect -> new Defect.Default(
...
Severity.WARNING,
...
)Context can be empty in Defect.Default (see the ctor without context param). About version - you should not set it, it will be automatically resolved in the .version() method of the Defect's implementation.
You can omit other parameters: line and program , while creating them in MarkdownLinter. You should be able to create new defect without those parameters in MarkdownLinter first, and then, in LtInvalidMarkdownComment create Defect with those parameters being set.
All in all, my point is that instead of MarkdownDefect, we can use Defect.Simple in two-steps, but probably we should extend Defect.Simple ctors. WDYT?
|
|
||
| Comments in the code must be checked for correctness from the point of view of | ||
| [markdownlint](https://github.com/DavidAnson/markdownlint) | ||
| (excluding rules |
There was a problem hiding this comment.
@Marat-Tim its good to mention rules we exclude. However, its better to move this text at the bottom of motive
| [MD026](https://github.com/DavidAnson/markdownlint/blob/main/doc/md026.md) | ||
| ) | ||
|
|
||
| Incorrect: |
There was a problem hiding this comment.
@Marat-Tim let's add an output, that mdlint will give us for "incorrect" EO snippet
| Incorrect: | ||
|
|
||
| ```eo | ||
| # # Main object. |
There was a problem hiding this comment.
@Marat-Tim to be honest, its not really catchy example of "incorrect" code. Let's use different example to demonstrate that markdown comments are broken. How about this:
# * Item 1
# * Nested Item 1
# * Nested Item 2
# * A misaligned item
[] > foo
This is the violation of MD005
There was a problem hiding this comment.
@h1alexbel This seems to be a problem, in the xmir of this program, such a comment looks like this
<comments>
<comment line="5">* Item 1\n* Nested Item 1\n* Nested Item 2\n* A misaligned item</comment>
</comments>| /** | ||
| * Test for {@link LtMarkdownOnly}. | ||
| * | ||
| * @since 0.57.0 |
|
@Marat-Tim also, when you create PR to "close" some issue, mention it in the PR description, please |
Marat-Tim
changed the title
markdownlint for check commentsmarkdownlint for check comments
|
@h1alexbel I've corrected all the comments except deleting MarkdownDefect. Can you clarify exactly whether it needs to be replaced with Defect.Default? And if so, what values should I put in the severity, program, and version fields? Should I set them to null? |
h1alexbel
left a comment
h1alexbel
left a comment
There was a problem hiding this comment.
@Marat-Tim please check my comments
| <version>9.8</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
@Marat-Tim let's move these dependencies upper, before test scope libraries
| * | ||
| * @since 0.57.0 | ||
| */ | ||
| public interface MarkdownDefect { |
There was a problem hiding this comment.
@Marat-Tim severity is the same you specify here (WARNING):
defect -> new Defect.Default(
...
Severity.WARNING,
...
)Context can be empty in Defect.Default (see the ctor without context param). About version - you should not set it, it will be automatically resolved in the .version() method of the Defect's implementation.
You can omit other parameters: line and program , while creating them in MarkdownLinter. You should be able to create new defect without those parameters in MarkdownLinter first, and then, in LtInvalidMarkdownComment create Defect with those parameters being set.
All in all, my point is that instead of MarkdownDefect, we can use Defect.Simple in two-steps, but probably we should extend Defect.Simple ctors. WDYT?
|
@h1alexbel Is using null in such a context acceptable? Of course, we can replace it with empty strings, but still, if someone uses this data, they will get a strange result |
| final String rule, final Severity severity, | ||
| final int line, final String text | ||
| ) { | ||
| this(rule, severity, null, line, text); |
There was a problem hiding this comment.
@Marat-Tim bad idea to use NULL. Let's use empty string. Also, read this: https://www.yegor256.com/2014/05/13/why-null-is-bad.html
There was a problem hiding this comment.
@Marat-Tim Let's make this ctor private for end-users as well
| * | ||
| * @since 0.0.47 | ||
| */ | ||
| public final class MarkdownLinter implements Closeable { |
| this.context.close(); | ||
| } | ||
|
|
||
| private static String getName(final Value names) { |
src/main/javascript/markdownlint.js
Outdated
| * SPDX-FileCopyrightText: Copyright (c) 2016-2025 Objectionary.com | ||
| * SPDX-License-Identifier: MIT | ||
| */ | ||
|
|
| * SPDX-FileCopyrightText: Copyright (c) 2016-2025 Objectionary.com | ||
| * SPDX-License-Identifier: MIT | ||
| */ | ||
|
|
| @@ -0,0 +1,17 @@ | |||
| /* | |||
There was a problem hiding this comment.
@Marat-Tim let's add puzzle to integrate eslint into repository
h1alexbel
left a comment
h1alexbel
left a comment
There was a problem hiding this comment.
@Marat-Tim I think these comments are last
| } | ||
|
|
||
| /** | ||
| * Get the first name starting with MD from the name array. |
There was a problem hiding this comment.
@Marat-Tim let's update JavaDoc as well, in order to get rid of "Get first name...". Can be simplified just to "MD rule"
|
@yegor256 take a look, please |
WalkthroughThis change introduces automated Markdown linting for EO language comments using markdownlint via GraalVM JavaScript integration. It adds new Java classes for linting, updates Maven and JavaScript build configurations to support Node.js and markdownlint, provides documentation and tests for the new lint rule, and integrates the check into the existing lint workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant EOFile as EO XML File(s)
participant LtInvalidMarkdownComment as LtInvalidMarkdownComment
participant MarkdownLinter as MarkdownLinter (GraalVM JS)
participant JS as markdownlint.js
EOFile->>LtInvalidMarkdownComment: Provide XML with comments
LtInvalidMarkdownComment->>MarkdownLinter: Initialize linter
loop For each comment in all files
LtInvalidMarkdownComment->>MarkdownLinter: defects(comment text)
MarkdownLinter->>JS: lint(comment text)
JS-->>MarkdownLinter: Linting results (violations)
MarkdownLinter-->>LtInvalidMarkdownComment: Defect(s) for violations
end
LtInvalidMarkdownComment-->>EOFile: Collected defects for all comments
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
![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
🧹 Nitpick comments (2)
src/test/java/org/eolang/lints/LtInvalidMarkdownCommentTest.java (1)
60-61: Fix assertion message to match test intent.The assertion message is misleading for this test case which expects no violations.
- "markdownlint found violation of its rules in comments", + "markdownlint should not find violations in correct markdown comments",src/main/java/org/eolang/lints/LtInvalidMarkdownComment.java (1)
30-37: Document why these markdownlint rules are ignored.Add documentation explaining the rationale for ignoring these specific rules.
/** * Markdownlint rule names to ignore. + * MD041: First line in file should be a top level heading (not applicable to comments) + * MD047: Files should end with a single newline character (not applicable to comments) + * MD026: Trailing punctuation in heading (too restrictive for comments) */ private static final Set<String> IGNORED = Set.of(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pom.xml(2 hunks)src/main/java/org/eolang/lints/Defect.java(1 hunks)src/main/java/org/eolang/lints/LtInvalidMarkdownComment.java(1 hunks)src/main/java/org/eolang/lints/MarkdownLinter.java(1 hunks)src/main/java/org/eolang/lints/WpaLints.java(1 hunks)src/main/javascript/markdownlint.js(1 hunks)src/main/javascript/package.json(1 hunks)src/main/javascript/webpack.config.js(1 hunks)src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md(1 hunks)src/test/java/org/eolang/lints/LtInvalidMarkdownCommentTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/org/eolang/lints/LtInvalidMarkdownCommentTest.java (1)
src/test/java/matchers/DefectMatcher.java (1)
DefectMatcher(20-51)
🔇 Additional comments (11)
src/main/java/org/eolang/lints/WpaLints.java (1)
29-30: LGTM! Clean integration following existing patterns.The addition of
LtInvalidMarkdownCommentto the lint list follows the established pattern and correctly integrates the new markdown comment validation into the WPA linting framework.src/main/javascript/package.json (1)
1-15: No known vulnerabilities in markdownlint ≥0.37.4Based on our verification, there are no publicly documented CVEs or security advisories affecting markdownlint version 0.37.4 or later. Your use of
"markdownlint": "^0.37.4"is considered safe under standard usage.• If you define custom rules that invoke third-party parsers (e.g. markdown-it), ensure those dependencies remain up-to-date and free of known issues.
• For ongoing assurance, consider adding an automated dependency/vulnerability scanner to your CI pipeline.src/main/javascript/markdownlint.js (2)
6-8: Well-structured TODO comment for ESLint integration.The TODO comment correctly identifies the need for ESLint integration to monitor JavaScript code quality, which aligns with the repository's code quality standards.
11-20: Clean and efficient markdownlint integration.The global
lintfunction implementation is well-structured:
- Uses synchronous markdownlint API appropriate for Java integration
- Properly structures the options object
- Returns results specific to the input text
The implementation appears to address previous review comments about code cleanliness.
src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md (1)
1-104: Comprehensive and well-structured documentation.The documentation effectively explains the lint rule with:
- Clear examples of violations and corrections using EO syntax
- Expected markdownlint output for each violation
- Explicit listing of ignored rules for transparency
- Proper references to external markdownlint documentation
The examples cover common markdownlint issues (MD001, MD004, MD018) that developers are likely to encounter.
src/main/java/org/eolang/lints/Defect.java (1)
161-174: Well-implemented constructor addressing previous review feedback.The new package-private constructor correctly:
- Uses empty string
""instead of null as recommended in past reviews- Maintains package-private visibility for controlled access
- Properly delegates to the existing 5-parameter constructor
- Follows established patterns in the codebase
This provides a clean API for creating defects without object names, which is appropriate for the markdownlint integration.
src/main/javascript/webpack.config.js (1)
8-24: LGTM!The webpack configuration is well-structured for bundling markdownlint for GraalVM JavaScript integration. The
webworkertarget and disabled Node.js core module fallbacks are appropriate choices for this environment.src/test/java/org/eolang/lints/LtInvalidMarkdownCommentTest.java (1)
21-56: LGTM!The test correctly validates detection of markdownlint violations, specifically testing for MD001 (heading level increment) errors.
pom.xml (1)
148-158: LGTM!GraalVM JavaScript dependencies are correctly configured with appropriate scopes.
src/main/java/org/eolang/lints/MarkdownLinter.java (1)
22-103: LGTM!Well-implemented GraalVM JavaScript integration with proper resource management and error handling. The URL polyfill is a clever solution for GraalVM limitations.
src/main/java/org/eolang/lints/LtInvalidMarkdownComment.java (1)
44-91: LGTM!Efficient implementation that properly reuses the MarkdownLinter instance across multiple files and correctly calculates line numbers for multi-line comments.
| <configuration> | ||
| <arguments>install</arguments> | ||
| <workingDirectory>${project.build.directory}/javascript</workingDirectory> | ||
| <installDirectory>.</installDirectory> |
There was a problem hiding this comment.
Change installDirectory to avoid polluting project root.
Setting installDirectory to . will install Node.js in the project root, which could lead to unwanted files in version control.
- <installDirectory>.</installDirectory>
+ <installDirectory>${project.build.directory}</installDirectory>📝 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.
| <installDirectory>.</installDirectory> | |
| <installDirectory>${project.build.directory}</installDirectory> |
🤖 Prompt for AI Agents
In pom.xml at line 734, the installDirectory is set to '.', which installs
Node.js in the project root and may clutter version control. Change the
installDirectory to a dedicated subdirectory (e.g., 'node' or 'target/node')
within the project to isolate installed files and avoid polluting the root
directory.
|
@Marat-Tim somehow, tests are failing here. If you can fix them, we'll merge |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md (3)
1-5: Suppress the Vale “markdownlint” false-positive once, rather than ignoring every CI run
markdownlintis a proper noun, but Vale’s default dictionary does not know it, which currently fails the pipeline.
Add the term to the project dictionary (e.g..vale/styles/accepted-words.txt) or create a local Vale rule override so we don’t have to fight the checker in the future.
18-20: Trim trailing whitespace to keep the file diff-friendlyThere are two lines with a single trailing space at EOL (
Sub information about main object.).
Those appear in many editors as visual noise and generate unnecessary git churn.Diff to apply:
-# Sub information about main object.␠ +# Sub information about main object.Also applies to: 37-39
8-9: Optionally silence heading-capitalisation warnings for rule IDsVale warns that headings like
MD001/MD004/MD018do not use sentence-style capitalisation.
Because these headings are literal rule identifiers, the warning is not actionable.
Consider adding a<!-- vale off --> … <!-- vale on -->wrapper around those rule-ID headings or tweaking.vale.inito ignoreMicrosoft.Headings/Google.Headingsfor this file.Also applies to: 42-43, 73-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
pom.xml(3 hunks)src/main/java/org/eolang/lints/Defect.java(1 hunks)src/main/java/org/eolang/lints/LtInvalidMarkdownComment.java(1 hunks)src/main/java/org/eolang/lints/MarkdownLinter.java(1 hunks)src/main/java/org/eolang/lints/WpaLints.java(1 hunks)src/main/javascript/markdownlint.js(1 hunks)src/main/javascript/package.json(1 hunks)src/main/javascript/webpack.config.js(1 hunks)src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md(1 hunks)src/test/java/org/eolang/lints/LtInvalidMarkdownCommentTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/eolang/lints/LtInvalidMarkdownComment.java
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/java/org/eolang/lints/WpaLints.java
- src/main/javascript/markdownlint.js
- src/main/java/org/eolang/lints/Defect.java
- src/main/javascript/package.json
- src/main/javascript/webpack.config.js
- src/test/java/org/eolang/lints/LtInvalidMarkdownCommentTest.java
- src/main/java/org/eolang/lints/MarkdownLinter.java
- pom.xml
🧰 Additional context used
🪛 GitHub Check: vale
src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md
[warning] 73-73:
[vale] reported by reviewdog 🐶
[Microsoft.Headings] 'MD018' should use sentence-style capitalization.
Raw Output:
{"message": "[Microsoft.Headings] 'MD018' should use sentence-style capitalization.", "location": {"path": "src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md", "range": {"start": {"line": 73, "column": 6}}}, "severity": "INFO"}
[warning] 73-73:
[vale] reported by reviewdog 🐶
[Google.Headings] 'MD018' should use sentence-style capitalization.
Raw Output:
{"message": "[Google.Headings] 'MD018' should use sentence-style capitalization.", "location": {"path": "src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md", "range": {"start": {"line": 73, "column": 6}}}, "severity": "WARNING"}
[warning] 42-42:
[vale] reported by reviewdog 🐶
[Microsoft.Headings] 'MD004' should use sentence-style capitalization.
Raw Output:
{"message": "[Microsoft.Headings] 'MD004' should use sentence-style capitalization.", "location": {"path": "src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md", "range": {"start": {"line": 42, "column": 6}}}, "severity": "INFO"}
[warning] 42-42:
[vale] reported by reviewdog 🐶
[Google.Headings] 'MD004' should use sentence-style capitalization.
Raw Output:
{"message": "[Google.Headings] 'MD004' should use sentence-style capitalization.", "location": {"path": "src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md", "range": {"start": {"line": 42, "column": 6}}}, "severity": "WARNING"}
[warning] 8-8:
[vale] reported by reviewdog 🐶
[Microsoft.Headings] 'MD001' should use sentence-style capitalization.
Raw Output:
{"message": "[Microsoft.Headings] 'MD001' should use sentence-style capitalization.", "location": {"path": "src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md", "range": {"start": {"line": 8, "column": 6}}}, "severity": "INFO"}
[warning] 8-8:
[vale] reported by reviewdog 🐶
[Google.Headings] 'MD001' should use sentence-style capitalization.
Raw Output:
{"message": "[Google.Headings] 'MD001' should use sentence-style capitalization.", "location": {"path": "src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md", "range": {"start": {"line": 8, "column": 6}}}, "severity": "WARNING"}
[failure] 4-4:
[vale] reported by reviewdog 🐶
[Vale.Spelling] Did you really mean 'markdownlint'?
Raw Output:
{"message": "[Vale.Spelling] Did you really mean 'markdownlint'?", "location": {"path": "src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md", "range": {"start": {"line": 4, "column": 2}}}, "severity": "ERROR"}
[failure] 1-1:
[vale] reported by reviewdog 🐶
[Vale.Spelling] Did you really mean 'markdownlint'?
Raw Output:
{"message": "[Vale.Spelling] Did you really mean 'markdownlint'?", "location": {"path": "src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md", "range": {"start": {"line": 1, "column": 26}}}, "severity": "ERROR"}
🪛 GitHub Actions: vale
src/main/resources/org/eolang/motives/comments/invalid-markdown-comment.md
[error] 1-1: [Vale.Spelling] Did you really mean 'markdownlint'?
[error] 4-4: [Vale.Spelling] Did you really mean 'markdownlint'?
[warning] 8-8: [Google.Headings] 'MD001' should use sentence-style capitalization.
[warning] 42-42: [Google.Headings] 'MD004' should use sentence-style capitalization.
[warning] 73-73: [Google.Headings] 'MD018' should use sentence-style capitalization.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: mvn (windows-2022, 11)
- GitHub Check: mvn (macos-15, 11)
- GitHub Check: mvn (macos-15, 21)
- GitHub Check: mvn (windows-2022, 21)
- GitHub Check: ort
- GitHub Check: mvn (ubuntu-24.04, 21)
- GitHub Check: mvn (ubuntu-24.04, 11)
- GitHub Check: jmh
- GitHub Check: deep
3 participants
In this pull request, I propose to solve the problem of verifying the validity of comments from the markdown point of view by connecting the markdownlint tool and creating a bridge between java and javascript. Although the running time is very long, it seems to me that this is the best option, since we cannot write and maintain our own library for checking markdown. Also, using outdated tools like oxygenxml doesn't sound good either. Using a bridge between java and javascript will allow us to use the supported markdownlint library as well as the ability to run other js tools in our code.
Summary by CodeRabbit
New Features
Documentation
Tests