feat: support line number in fragment in MD051/link-fragments (e.g: '#L7') #1117
Conversation
|
Test Repos seems failing by unrelated changes. As the errors are: test-repos/v8-v8-dev/src/features/import-attributes.md: 59: MD034/no-bare-urls Bare URL used [Context: "https://chromestatus.com/featu..."]␊
- test-repos/v8-v8-dev/src/features/import-attributes.md: 61: MD034/no-bare-urls Bare URL used [Context: "https://developer.apple.com/do..."]␊
- test-repos/v8-v8-dev/src/features/import-attributes.md: 63: MD034/no-bare-urls Bare URL used [Context: "https://babeljs.io/blog/2023/0..."]␊Which MD034 is not updated with this PR. |
|
Cool, thanks, I'll start a review in a moment! A couple of things:
|
|
There was a problem hiding this comment.
Makes sense to me! :) Please let me know if the simplification I propose works - I think it cuts out a lot of this code. Also, please mention the rule allows "#L123" because of GitHub in the docs. doc-build/md051.md is the file to edit and everything else will get generated by npm run ci.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Yes. It is documented here: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/creating-a-permanent-link-to-a-code-snippet#linking-to-markdown |
theoludwig
changed the title
|
The GitHub "specification" is pretty light on detail, but it was enough to remind me this syntax is more capable than just a single line number. For example, the following represents the full form I'm aware of which links to starting line/column and ending line/column: So the regular expression should be something more like |
doc-build/md051.md
Outdated
| @@ -38,6 +38,14 @@ attribute can be used to define a fragment: | |||
| An `a` tag can be useful in scenarios where a heading is not appropriate or for | |||
| control over the text of the fragment identifier. | |||
|
|
|||
| GitHub also supports links to [specific lines][github-lines-links]. | |||
|
|
|||
| For example, to link to line 12 of current Markdown file: | |||
There was a problem hiding this comment.
I'm not sure I want to suggest doing this because GitHub also says you need to do plain=1. I'd say let's say something like "That syntax looks like this is practice:".
lib/md051.js
Outdated
| continue; | ||
| } | ||
|
|
||
| if (encodedText.startsWith('#L')) { |
There was a problem hiding this comment.
I don't understand this part. Why does this need special handling? I'd assume if this were not done then we could move the lineFragmentsRe.test into the if below along with the other checks as I originally proposed. There was no lowercase conversion before, so I think there should not be one after. Maybe this gets to the other point about case sensitivity - I consider the "L111" form to be the only allowed form by this rule since that's what GitHub produces. Anyone using "l111" should switch - which will help avoid confusing letters and numbers. :)
There was a problem hiding this comment.
Without this condition, the following test case fails:
### L12 ABC
[Valid](#L12-abc)From what I understand, with your previous lowercase related comment, it is to be expected, as the link should have been [Valid](#l12-abc) as it is a heading.
But when it's about #L12, the L should be uppercase.
Looks fine to me, let's be stricter than GitHub about case sensitivity. But we should maybe document it in md051.md? As it might be unexpected for users (me included) that the rule reports errors for valid fragments because of stricter case sensitivity management.
Will simplify the code and make the necessary changes whenever I have time. 👍
|
@DavidAnson Thank you for reviewing the PR. 😄 I should have addressed most feedbacks from reviews, notably:
|
|
This looks good, thank you! I think I want to iterate on the documentation a little after thinking about it some more - I'll do that tomorrow by pushing to this branch and then accept the PR. |
|
For posterity since I tried the combinations out, these are all the possibilities and how GitHub handles them: HIGHLIGHT NO HIGHLIGHT IGNORED |
|
... and this regular expression matches ONLY the top group above: |
… syntax in URLs (refs #1117).
…sts, and documentation.
… syntax in URLs (refs #1117).
Hello! 😄
I'm the original author of the rule MD051, work started in this PR: #495.
I'm also the author of https://github.com/theoludwig/markdownlint-rule-relative-links custom rule.
This custom rule, verify if relative links exists in the file system using Node.js
node:fs, it also checks for links fragments referencing other files (similar to the rule MD051, by trying to have as much as possible the same behavior as the MD051 built-in rule in this regard).GitHub supports links fragments for referring to a specific line, e.g:
#L7means "Go To Line 7`.More context: theoludwig/markdownlint-rule-relative-links#6
Currently MD051 reports an error for that kind of fragments, but it should not, as they are valid on GitHub.
This PR fixes the issue, similarly to how it is done in
markdownlint-rule-relative-links.In the custom rule, we can go even further and check if the file has enough lines to be a valid line number reference (e.g:
#L7in a empty file, is invalid, as line 7 doesn't exist), but that needs access to Node.jsnode:fs, which as far as I understand, you don't want to use for built-in rule for compatibility with the browser. So for now we're just considering it's valid without extra checks.Feel free to suggest ideas, suggest better implementation, or maybe you have ideas of more test cases to add?
Thanks!