-
Notifications
You must be signed in to change notification settings - Fork 63
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
feat: Add Markdown languages #268
Conversation
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.
Nice work! I noted a few things we may want to get fixed before releasing the changes.
src/rules/no-html.js
Outdated
return; | ||
} | ||
|
||
const tagName = node.value.match(/<([a-zA-Z0-9]+)/u)?.[1]; |
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.
This will not correctly match custom tag names with non-alphanumeric characters, if someone accidentally inserts them in the markdown. For example:
<ng-template>
Hello, World!
</ng-template>
Will give the error:
HTML element "ng" is not allowed.
I'm not sure what tag names are considered valid by parsers, but I think it would be better to make the regexp more permissive. Maybe like this?
const tagName = node.value.match(/<([a-zA-Z0-9]+)/u)?.[1]; | |
const tagName = node.value.match(/<([^\/>\s]+)/u)?.[1]; |
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.
Good point. I'll need to test the parser a bit to see what happens.
"[foo][]\n\n[foo]: http://bar.com/image.jpg", | ||
"![foo][]\n\n[foo]: http://bar.com/image.jpg" |
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.
It looks like GitHub is trimming spaces from within brackets, and even a single line break is ignored. So the following should be all valid equivalents of [foo][]\n\n[foo]: http://bar.com/image.jpg
but they are being reported as errors:
"[ foo ][]\n\n[foo]: http://bar.com/image.jpg",
"[foo][ ]\n\n[foo]: http://bar.com/image.jpg",
"[\nfoo\n][\n]\n\n[foo]: http://bar.com/image.jpg",
I'm not sure if this is different in standard CommonMark.
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 the heads up. I'll investigate.
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've tested this with the parser in both CommonMark and GFM modes.
For the first one, it correctly parses in both modes with the label of "foo", so it's trimming the label. The rule is not flagging this as an error.
The second and third are not being parsed as link references in either CommonMark or GFM mode. When I remove the whitespace between the [
and ]
, then they are recognized as link references. This appears to be a bug in the parser. I'll need to follow up with them, but I can still flag it in the rule.
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.
According to the parser maintainers, []
with any amount of whitespace in between is not supposed to create a link. It seems that CommonMark forbids this and the fact that it's working on GitHub is actually a bug in GitHub's implementation.
So really, only the first example is a valid link. The other two are not considered links and therefore shouldn't check the link reference. I'll need to update accordingly.
We probably also need a no-sloppy-link-ref
rule to flag those as not compatible with CommonMark.
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 digging into this. The discussion in the mdast-util-from-markdown
is helpful. If it's true that the different behavior in GFM is a bug and not the specified behavior then we can use probably https://spec.commonmark.org/dingus/ as a references to test how links should regardless of the markdown language.
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
Co-authored-by: Francesco Trotta <[email protected]>
Added a new |
I updated |
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 this rule, the column calculation is incorrect when the link starts after the beginning of the line. For example with this test case:
{
code: "- - - [foo]",
errors: [
{
messageId: "notFound",
data: { label: "foo" },
line: 1,
column: 8,
endLine: 1,
endColumn: 11
}
]
}
This is similar to the issue we had with no-invalid-label-refs
.
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.
Ah right, same logic. Thanks.
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.
Out of curiosity, I run the new rules against the .md
files in https://github.com/eslint/eslint.org/. This reported a few hundred violations, some of them a bit unexpected like no-missing-label-refs
on [object Object]
. But on careful look, those were all legitimate violations. Duplicate headings are also very common and probably not something we want to address. Overall, I think this is working well and it's a useful reference for how to add language capability to a plugin.
src/rules/no-invalid-label-refs.js
Outdated
|
||
/* | ||
* Search the entire document text to find the preceding open bracket. | ||
* We add one to the start index to account for a preceding ! in an image. |
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.
We are no longer adding one.
Just the above note, then LGTM. |
Thanks! Yeah |
This pull request adds two markdown languages to the plugin:
commonmark
- CommonMark syntaxgfm
- GitHub-Flavored Markdown syntaxIt also adds several rules that apply to Markdown content. These are all intended to find problems and not to format the document. I've included documentation for each rule.
I changed the
recommended
config to be for linting Markdown content, not using the processor. There is a newprocessor
config that implements the oldrecommended
config.Note on types: Because the
@types/eslint
package defines types specific to ESTree, I can't really add more types into the rules until we have a more generic type definition for rules. I useRuleModule
to ensure that the plugin itself will type check correctly, but that definition doesn't allow visitor methods for non-ESTree values.Refs #160