feat: add checkSiblingsOnly option to no-duplicate-headings rule #393
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new siblingsOnly option to the no-duplicate-headings rule so that duplicate heading checks may be applied only among headings that share the same parent.
- Added a siblingsOnly boolean option in the rule’s schema and updated the implementation to track headings by level.
- Updated tests and documentation to verify the behavior for ATX and setext-style headings.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/rules/no-duplicate-headings.test.js | New test cases validating the siblingsOnly behavior for both valid and invalid heading structures. |
| src/rules/no-duplicate-headings.js | Modified duplicate heading tracking logic to support the siblingsOnly option using level-based tracking instead of a global set. |
| docs/rules/no-duplicate-headings.md | Updated documentation with options and examples for the new siblingsOnly setting. |
src/rules/no-duplicate-headings.js
Outdated
| if (headings.has(text)) { | ||
| const headingText = getHeadingText(node); | ||
|
|
||
| if (siblingsOnly) { |
There was a problem hiding this comment.
When siblingsOnly is false the rule should check for duplicate headings across the entire document. However, the current implementation always uses the level 1 array (currentLevelHeadings) to track headings, which can cause duplicate headings from different levels to be missed. Consider adding a separate global tracking set for the non-siblingsOnly case.
Copilot uses AI. Check for mistakes.
xbinaryx
changed the title
| function getHeadingText(node) { | ||
| /* | ||
| * There are two types of headings in markdown: | ||
| * - ATX headings, which start with one or more # characters | ||
| * - Setext headings, which are underlined with = or - | ||
| * Setext headings are identified by being on two lines instead of one, | ||
| * with the second line containing only = or - characters. In order to | ||
| * get the correct heading text, we need to determine which type of | ||
| * heading we're dealing with. | ||
| */ | ||
| const isSetext = | ||
| node.position.start.line !== node.position.end.line; | ||
|
|
||
| return isSetext | ||
| ? // get only the text from the first line | ||
| sourceCode.lines[node.position.start.line - 1].trim() | ||
| : // get the text without the leading # characters | ||
| sourceCode | ||
| .getText(node) | ||
| .slice(node.depth + 1) | ||
| .trim(); | ||
| } |
There was a problem hiding this comment.
Could you add logic to handle closed ATX headings and add corresponding test cases?
Currently, getHeadingText gives a false negative when using closed-style ATX headings.
For example:
## installation
## installation ##or
## installation
## installation ##########The second one is an invalid case.
https://spec.commonmark.org/0.31.2/#atx-headings
| if (checkSiblingsOnly) { | ||
| const currentLevel = node.depth; | ||
|
|
||
| const direction = currentLevel > lastLevel ? 1 : -1; |
There was a problem hiding this comment.
| const direction = currentLevel > lastLevel ? 1 : -1; | |
| const direction = currentLevel > lastLevel ? 1 : -1; |
Could you give a name to the magic numbers 1 and -1?
Currently, it's somewhat unclear what these numbers are doing.
Maybe we can use a constant like this:
const DOWN = -1;
const UP = 1;Or, we can use more concrete one with symbols:
const DOWN = Symbol('down');
const UP = Symbol('up'):|
|
||
| const direction = currentLevel > lastLevel ? 1 : -1; | ||
| while (lastLevel !== currentLevel) { | ||
| if (direction > 0) { |
There was a problem hiding this comment.
| if (direction > 0) { | |
| if (direction > 0) { |
Like the above comment, could you clarify the conditional statement?
Maybe we can use if (direction === UP) or something similar.
| ? new Array(7).fill(null).map(() => new Set()) | ||
| : [null, new Set()]; |
There was a problem hiding this comment.
| ? new Array(7).fill(null).map(() => new Set()) | |
| : [null, new Set()]; | |
| ? new Array(7).fill(null).map(() => new Set()) | |
| : [null, new Set()]; |
Could you use a Map instead of an Array?
Currently, the leading null item in the array is unused and it causes some confusion.
Maybe we can use a Map like this:
new Map([
[1, new Set()],
[2, new Set()],
[3, new Set()],
[4, new Set()],
[5, new Set()],
[6, new Set()]
])
Prerequisites checklist
What is the purpose of this pull request?
This PR adds a new siblingsOnly option to the no-duplicate-headings rule to make it more flexible. Currently, the rule flags any duplicate headings in the entire document, which can be too restrictive in some cases. With this new option, users can configure the rule to only check for duplicate headings among siblings (headings at the same level under the same parent heading).
What changes did you make? (Give an overview)
Added a new siblingsOnly boolean option to the rule's schema and refactored the implementation to track headings by level, resetting tracking when moving between levels when siblingsOnly is enabled. Updated documentation and added test cases for both ATX and setext-style headings to verify the new behavior.
Related Issues
Fixes #390
Is there anything you'd like reviewers to focus on?