-
Notifications
You must be signed in to change notification settings - Fork 80
feat: create no-unused-definitions rule
#425
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
Conversation
no-unused-definition ruleno-unused-definitions rule
…reate-no-unused-definition-rule
…b.com/eslint/markdown into feat-create-no-unused-definition-rule
|
|
||
| defaultOptions: [ | ||
| { | ||
| allowDefinitions: ["//"], |
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.
| allowDefinitions: ["//"], | |
| allowDefinitions: ["//"], |
// is usally used as a definition-style comment.
References:
- feat: create
no-duplicate-definitions#360 (comment) - https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md#md053---link-and-image-reference-definitions-should-be-needed
- https://stackoverflow.com/questions/4823468/comments-in-markdown
- https://www.jamestharpe.com/markdown-comments/
| footnoteDefinitions.add(node); | ||
| }, | ||
|
|
||
| "root:exit"() { |
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.
| "root:exit"() { | |
| "root:exit"() { |
In this PR, unlike the no-duplicate-definitions rule, errors should be reported only after all definition and footnoteDefinition nodes have been collected, as Reference and Definition nodes may appear in any order within the document.
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.
Pull Request Overview
This PR introduces a new ESLint rule no-unused-definitions to detect and report unused Markdown reference and footnote definitions.
- Adds the rule implementation under
src/rules. - Provides a comprehensive test suite covering valid/invalid cases.
- Updates documentation and the main README to include the new rule.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/rules/no-unused-definitions.test.js | Adds valid/invalid test cases for the new rule |
| src/rules/no-unused-definitions.js | Implements the no-unused-definitions rule logic |
| docs/rules/no-unused-definitions.md | Documents the rule’s behavior, options, and examples |
| README.md | Registers the rule in the plugin’s rule table |
Comments suppressed due to low confidence (1)
docs/rules/no-unused-definitions.md:19
- [nitpick] Consider replacing the inline HTML comment with a proper Markdown lint directive or moving the note outside the blockquote to improve readability and maintain consistency.
> [!IMPORTANT] <!-- eslint-disable-line -- This should be fixed in https://github.com/eslint/markdown/issues/294 -->
nzakas
left a comment
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.
Just one note on the docs and otherwise LGTM. Would like another review before merging.
Co-authored-by: Nicholas C. Zakas <[email protected]>
snitin315
left a comment
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.
LGTM, thanks!
Co-authored-by: Nitin Kumar <[email protected]>
Prerequisites checklist
What is the purpose of this pull request?
In this PR, I've created
no-unused-definitionsrule.I've left a few comments on some key parts.
This PR resolves #424.
What changes did you make? (Give an overview)
I've created
no-unused-definitionsrule.Related Issues
Fixes: #424
Is there anything you'd like reviewers to focus on?