Skip to content
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

Markdownlint: Add custom rule "Blanks around multiline HTML tags" #27674

Conversation

MaoShizhong
Copy link
Contributor

@MaoShizhong MaoShizhong commented Mar 23, 2024

Because

Discovered in #27648, until a blank line is reached, any text following an opening HTML tag gets ignored by the linter almost all of the time. This allows for easy misses due to uncaught rule errors.

The cause of this is markdown-it's method for parsing HTML blocks, where any content following an opening HTML tag, until a blank line is reached, gets bundled into the same single html_block token. Thus, they don't get their own tokens and so bypass lint checks.

By enforcing blank lines (or code block delimiters) surrounding isolated HTML tags, the above is avoided and all HTML block text content can be linted appropriately. HTML tags within text (e.g. spans/anchors for link fragments) are ignored by this rule.

This PR

  • Adds a new custom rule TOP005
  • Adds a test .md file for TOP005 demonstrating valid and invalid cases
  • Adds a TOP005 documentation file
  • Adds a TOP005 file path to the linter config file's custom rules property
  • Disables the linter for the first line of all custom rule doc files, which require a level 1 heading instead of a level 3 heading (as those files will not be rendered on the TOP website)

Issue

Closes #27673

Additional Information

Failed lint checks all come from the TOP005_test.md file which are all intended errors.

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

Original function would flag markdownlint-disable-* comments which would
require being followed by the line to disable.
It would also flag HTML tags directly following or preceding triple
backticks. These are now allowed so that code blocks can start and end
more cleanly.
Added TOP005 rule to linter config file.
These files' first lines need to be level 1 headings, not level 3, as
they are not files to be rendered on the TOP website.
@MaoShizhong
Copy link
Contributor Author

MaoShizhong commented Mar 23, 2024

I like how it already includes the new rule in the actions. I guess it runs using the rules from my fork as opposed to the rules from upstream.
Nice thing is it makes it much easier to see how the rule does and doesn't work in the TOP005_test.md file.

Technically HTML tags on the first or last line of a file would trigger
other rules to error, but adding `?? ""` for the sake of completeness.

Single quotes have been switched to double quotes to be consistent with
other JS files in the repo.
Demonstrates how a lack of blank lines interferes with unrelated lint
checks.
@MaoShizhong MaoShizhong marked this pull request as draft March 25, 2024 12:41
@MaoShizhong
Copy link
Contributor Author

Bug found - need to account for any unindented HTML tags inside code blocks and not the first or last line. Rare edge case but would rather fix it now than later.

These cases have a legitimate need to "break the rule".
We only want to flag for HTML tags we use for the actual lesson markup,
or md code block examples of such.

.trim added so that indentation doesn't matter.
Now includes examples of ignored code blocks (html/jsx type).
@MaoShizhong MaoShizhong marked this pull request as ready for review March 25, 2024 16:25
@MaoShizhong
Copy link
Contributor Author

Bug fixed - needed to allow all HTML within html/jsx code blocks.
Outside of these two code block types, the rule is unchanged.

MaoShizhong and others added 2 commits March 30, 2024 14:55
Removed unrelated errors to make it easier to demonstrate TOP005.
The behaviour with unrelated errors are now explained in text.
Fixed opening introduction line as it incorrectly stated there would not
be any errors flagged.
Opted to return token.map as is on line 19 instead of the same but with 1
subtracted from token.map[1].
The only difference this makes is include the closing backticks line in
the isWithinIgnoredFence range check, which will have no effect on the
outcome of the check.
The benefit is not needing to explain why we need to subtract 1 from
token.map[1] only. This allows everything to remain intuitive, and the
only deviation being the onError.lineNumber value which has a comment to
explain its 1-index requirement.

Regexr test links added.

Co-authored-by: Eric Olkowski <thatblindgeye@gmail.com>
These should already be ignored by the lint actions
Includes visual formatting improvements.
@MaoShizhong MaoShizhong merged commit 293d555 into TheOdinProject:main Mar 30, 2024
2 of 3 checks passed
@MaoShizhong MaoShizhong deleted the feature/mdlint-custom-no-blanks-around-multiline-html branch March 30, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdownlint new rule: Multi-line HTML tags must be surrounded by blank lines
2 participants