-
-
Notifications
You must be signed in to change notification settings - Fork 82
feat: support GFM alert syntax parsing #138
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
base: main
Are you sure you want to change the base?
Conversation
I've tried to include many valid and invalid syntax examples, but there may be additional cases. I'm happy to add more examples. |
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.
Thanks for putting this together. The RFC is missing detailed design section, specifically:
- You mention
languageOptions
only in the Backwards Compatibility section. If this is your proposed approach to allow people to opt-in or opt-out of this behavior, this should be mentioned in the detailed design section. - Where would the implementation of these two proposed packages live? How would they be included or excluded?
|
||
The motivation for this RFC comes from [eslint/markdown#294](https://github.com/eslint/markdown/issues/294) and [eslint/markdown#449](https://github.com/eslint/markdown/issues/449). | ||
|
||
Currently, GitHub supports a special syntax for alerts that isn't mentioned in the [GitHub Flavored Markdown spec](https://github.github.com/gfm/) and isn't parsed by the existing [`micromark-extension-gfm`](https://github.com/micromark/micromark-extension-gfm?tab=readme-ov-file#when-to-use-this) and [`mdast-util-gfm`](https://github.com/syntax-tree/mdast-util-gfm?tab=readme-ov-file#when-to-use-this) packages, which are used internally by `@eslint/markdown` to parse Markdown content and create AST nodes. |
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.
GitHub alerts are not part of GFM. That’s why it’s not in the GFM spec, not supported by micromark-extension-gfm
, mdast-util-gfm
, etc. Instead, it’s an HTML transform applied to <blockquote>
elements after the markdown processing is done. Link references are just link references. Square brackets can be used to escape the link syntax.
The following markdown:
Just [some] text
> [!NOTE]
> A
> [!IMPORTANT]
> B
> \[!IMPORTANT]
> C
[!important]: https://example.com
Renders as:
Just [some] text
Note
A
Important
C
It’s similar to mermaid
code blocks. They are just code blocks. They get processed in the HTML transforms later.
IMO it does make sense to handle these cases in linting rules. It just doesn’t belong in the parser or the mdast.
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.
Thanks for your comment.
I'm aware that GitHub alert syntax isn't part of the official GFM specification.
I tried to address eslint/markdown#294 by adding an option (which seemed like the simplest fix), but after discussion the team (and me) concluded this should be handled by the parser and agreed to proceed that way.
Given the discussions in eslint/markdown#294, if you think that's the wrong direction, what do you think is the best way to resolve this?
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.
I would argue they are not false positives. The example in my previous comments shows that the existence of a definition affects how the GitHub alert is rendered.
I would resolve the issue by focusing on teaching, e.g. through documentation:
- GitHub alerts (not GFM alerts) are not part of the language. They’re an HTML transform done by a postprocessor.
- Although greater than signs (
>
) create blockquotes, GitHub replaces the<blockquote>
elements for alerts with<div>
elements. - Square brackets (
[
) may appear inside markdown text. - Square brackets (
[
) in markdown can be escaped using a backslash (\
) - Escaped square brackets still work for GitHub alerts.
IMO it’s good to always escape link square brackets to avoid ambiguousness. Opinions may vary of course.
If you insist on allowing these exceptions, I would handle them at the rule level. Also beware that GitHub alerts are only allowed at the root of the document.
> [!NOTE]
> Hello
> > [!NOTE]
> > Hello
- > [!NOTE]
> Hello
Note
Hello
[!NOTE]
Hello
-
[!NOTE]
Hello
Fun fact: This is a big reason why you can’t stream markdown. A definition at the end of a document may affect the meaning of unescaped square brackets earlier in 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.
Thanks for your detailed explanation — it was very helpful 👍
From my understanding, > [!IMPORTANT]
can be interpreted as a LinkReference
when there is a Definition
like [!important]: https://example.com
at the end of the document, so the no-missing-label-refs
rule recognizing it as an invalid LinkReference
is the correct behavior, not a false positive.
If my understanding is correct, I agree that it works as intended, since this is behavior compliant with the spec.
I also hadn't thought about that escaping the square brackets in GitHub alert syntax with backslashes would prevent them from being recognized as a valid LinkReference
(and rendered correctly as well).
I think @remcohaszing's opinion also makes sense to me as well. Could we get more input from @eslint/eslint-team?
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.
I'm not sure if asking end users to escape all alerts is a good solution for eslint/markdown#294 as it would look like an extra work just to appease the linter. From end user's perspective, > [!NOTE]
is a valid, meaningful code that behaves (renders) as expected, and reporting it as a missing ref looks like a false positive because it wasn't intended to be a ref.
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.
I can see that. Whether or not the [
should be escaped is a personal opinion. I hope my comments show that handling alert syntax at the parser level is technically incorrect. It was a deliberate choice to not support this in the unified ecosystem. I think it would be a mistake to extend mdast like that. It would be better to handle this at the rule level.
If you want to handle these cases, you could also consider if confusing link references should be reported as well.
> [!NOTE]
> Useful information that users should know, even when skimming content.
[!note]: https://example.com
!NOTE
Useful information that users should know, even when skimming content.
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.
In my opinion, marking alerts in the AST at language level would be less error-prone and avoid repetitions in rule logic. If this can be achieved with a parser extension, as proposed in this RFC, that would be probably the cleanest solution. Perhaps a prototype implementation would help to identify potential problems.
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.
@remcohaszing if you follow the original issue, we discussed at length how to address this issue and landed on promoting these alert boxes into the AST. While you are correct that this is technically not something the parser should be doing, when focused on the user experience (as @fasttime points out), this approach will produce the most reliable outcome for our users.
Appreciate everyone's reviews. I've added some more details to this RFC. |
|
||
## Help Needed | ||
|
||
I've been reviewing the `micromark-extension` and `mdast-util` guides and implementations, but I'm not sure how best to implement the `micromark-extension-gfm-alert` and `mdast-util-gfm-alert` plugins. |
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.
I think this the detail we need to really feel comfortable moving forward with this RFC. Right now, the RFC describes the desired outcome but not how to achieve it, and part of the RFC process is validating that the approach is possible. I'd encourage you to look through some existing plugins to see if you can come up with an approach that makes sense and maybe try prototyping.
You may want to take a look at the gfm-task-list plugins as an example:
https://github.com/micromark/micromark-extension-gfm-task-list-item/blob/main/dev/lib/syntax.js
Summary
This RFC proposes adding support for GFM alert syntax parsing to the
@eslint/markdown
package by implementingmicromark-extension-gfm-alert
andmdast-util-gfm-alert
custom plugin logic, and also includes the formal specification based on the CommonMark spec and the GitHub Flavored Markdown spec.Related Issues
Refs: eslint/markdown#294