fix(lint): apply suppression comments to relocated footnote definitions - #1142
Closed
VXNCXNX wants to merge 2 commits into
Closed
fix(lint): apply suppression comments to relocated footnote definitions#1142VXNCXNX wants to merge 2 commits into
VXNCXNX wants to merge 2 commits into
Conversation
goldmark hoists every footnote definition into a trailing div.footnotes, so the linear comment walk reached one after an enclosing = NO ... = YES pair had already flipped back and the alert was not suppressed. Fixes vale-cli#1078
Member
|
Thanks — the diagnosis is right, but I don't want to take this mechanism. It adds a second text-to-source heuristic beside the one the walker already uses for alert positions, mutates |
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
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.
Fixes #1078.
What's broken
Suppression comments do not suppress alerts inside footnote definitions.
The same document with the footnote replaced by a plain paragraph reports nothing.
The cause
Not what the issue reporter guessed. goldmark's Footnote extension relocates every definition into a trailing
div.footnotes, which I confirmed by dumping the rendered HTML:The toggle state is a linear walk over the token stream, so by the time the relocated text is reached, the
= YEShas already re-enabled the check. The telling detail is the converse: an unterminated= NOdoes suppress the footnote, which is only possible if position in the stream, not the footnote itself, is the variable.The fix
Inside that div, rebuild the state from the definition's own source line by replaying the comment controls that precede it, then restore the ambient state on the closing tag.
One deviation worth calling out: I key on the definition's source position, not the reference's. The issue's own example has the reference on line 1, outside the suppression block, with only the definition inside it. Keying on the reference leaves that example still reporting the alert. I built it that way first and confirmed it.
MyST and qmd
The
div.footnotescontainer is a goldmark promise, not a general one, so the branch is gated on.md,.mdx,.mystand.qmd. Those are exactly the four whose converters registerextension.Footnote, so rST, AsciiDoc and HTML never enter it. MyST and qmd are included because they relocate footnotes too and had the same bug; their existing tests pass unchanged.Verification
go build ./...clean andgo test ./internal/...passes in full.The MWE above now reports nothing and exits 0. Deleting the suppression comments brings the alert back.
The interesting case is this one, which broke my first attempt:
Resolving the definition by searching the whole document for its longest word finds line 1, so the replay stopped before the
= NOand the definition was not suppressed. It now walks the source's definition starts and picks the one whose body contains the word. Only the line-1 alert is reported, which is correct.Also checked: two footnotes where one is suppressed and one is not, and a definition whose longest word recurs in a later paragraph. Both behave.
One honest weakness.
f.ContentisprepMarkdown-processed, which stars out the label, so[^x]:becomes[**]:and the regex has to match definitions by shape rather than by\[\^. That means a link reference definition could in principle be selected first if it contained the same longest word. I could not construct a case where this produced a wrong suppression, since link definition labels are starred out and their URLs are not linted, but the ambiguity is there and I would rather you knew about it than found it.This touches the same walker branch as #1001, #1052 and #1111. The change is purely additive, three touch points in the loop and a self-contained type below
lintHTMLTokens, so it should not conflict badly.