-
Notifications
You must be signed in to change notification settings - Fork 80
fix: report locations with <CR> linebreaks in no-reference-like-urls
#525
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
Conversation
| endLine: 1, | ||
| endColumn: 20, | ||
| endLine: 2, | ||
| endColumn: 9, |
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.
Looks like there's a bug in findOffsets. Now that this rule is modified to report node location directly from the AST, lone <CR> increases line number.
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.
| endLine: 1, | ||
| endColumn: 21, | ||
| endLine: 2, | ||
| endColumn: 9, |
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.
Same as #525 (comment)
|
|
||
| /** Pattern to match both inline links: `[text](url)` and images: ``, with optional title */ | ||
| const linkOrImagePattern = | ||
| /(?<=(?<!\\)(?:\\{2})*)(?<imageBang>!)?\[(?<label>(?:\\.|[^()\\]|\([\s\S]*\))*?)\]\((?<destination>[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?:<[^>]*>|[^ \t()]+))(?:[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?<title>"[^"]*"|'[^']*'|\([^)]*\)))?[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*\)(?!\()/gu; |
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 what cases were being checked by (?!\() at the end, but no tests are failing now that I've removed it since the regex gets only the image/link node text so it can't check for a parenthesis after the image/link text.
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 also think it's safe to remove it, since I tried to find edge cases but couldn't detect any now that we're inspecting the link and image nodes directly.
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 taking a look! The updated logic looks great.
I've left a few minor suggestions and noted some false negatives in the regex to discuss.
| endLine: 1, | ||
| endColumn: 20, | ||
| endLine: 2, | ||
| endColumn: 9, |
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.
| function isInSkipRange(index, skipRanges) { | ||
| return skipRanges.some(range => range[0] <= index && index < range[1]); | ||
| } | ||
| /(?<imageBang>!)?\[(?<label>(?:\\.|[^()\\]|\([\s\S]*\))*?)\]\((?<destination>[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?:<[^>]*>|[^ \t()]+))(?:[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?<title>"[^"]*"|'[^']*'|\([^)]*\)))?[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*\)$/u; |
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.
[[](](link)
[[])](link)

![[])](image)According to the AST, the code above are valid links and images.
But, they produces false negatives like the following:
- test cases:
{
code: dedent`
[[](](mercury)
[mercury]: https://example.com/mercury
`,
output: dedent`
[[](][mercury]
[mercury]: https://example.com/mercury
`,
errors: [
{
messageId: "referenceLikeUrl",
data: { type: "link", prefix: "" },
line: 1,
column: 1,
endLine: 1,
endColumn: 15,
},
],
},
{
code: dedent`
[[])](mercury)
[mercury]: https://example.com/mercury
`,
output: dedent`
[[])][mercury]
[mercury]: https://example.com/mercury
`,
errors: [
{
messageId: "referenceLikeUrl",
data: { type: "link", prefix: "" },
line: 1,
column: 1,
endLine: 1,
endColumn: 15,
},
],
},
{
code: dedent`

[mercury]: https://example.com/mercury
`,
output: dedent`
](mercury)
[mercury]: https://example.com/mercury
`,
output: dedent`
![[])][mercury]
[mercury]: https://example.com/mercury
`,
errors: [
{
messageId: "referenceLikeUrl",
data: { type: "image", prefix: "!" },
line: 1,
column: 1,
endLine: 1,
endColumn: 16,
},
],
},- result:
The regex could probably be simplified further, but that would be a more complicated change.
But, I'm not sure this fix should be included in this PR.
As you mentioned above, would it be better to address this bug in a separate PR?
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.
But, they produces false negatives like the following
Is this a bug that this PR would introduce, or does it already exist in the current version of this rule in the main branch?
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 seems that this bug already exists. It also fails on the main branch.
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.
Then I think we could fix it in a separate PR as it's unrelated to this change (unless this change would make it more difficult to fix this bug and we'd need to revert it?). This change was meant to be just a refactor to simplify the rule, and the <CR> bug fix came as a side effect.
|
|
||
| /** Pattern to match both inline links: `[text](url)` and images: ``, with optional title */ | ||
| const linkOrImagePattern = | ||
| /(?<=(?<!\\)(?:\\{2})*)(?<imageBang>!)?\[(?<label>(?:\\.|[^()\\]|\([\s\S]*\))*?)\]\((?<destination>[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?:<[^>]*>|[^ \t()]+))(?:[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*(?<title>"[^"]*"|'[^']*'|\([^)]*\)))?[ \t]*(?:\r\n?|\n)?(?<![ \t])[ \t]*\)(?!\()/gu; |
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 also think it's safe to remove it, since I tried to find edge cases but couldn't detect any now that we're inspecting the link and image nodes directly.
Co-authored-by: 루밀LuMir <rpfos@naver.com>
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.
LGTM, thanks!
Would like one more review before merging.
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.
LGTM. Thanks.
Prerequisites checklist
What is the purpose of this pull request?
Refactors
no-reference-like-urlsto visitimageandlinknodes directly and thus simplify the code, and also fixes report location when the link/image contains lone<CR>as a line break (see the two updated test cases).What changes did you make? (Give an overview)
Refactored
no-reference-like-urlsto visitimageandlinknodes directly.Related Issues
Discussion #523 (comment)
Is there anything you'd like reviewers to focus on?
Parsing of link/image node texts is still needed because of the
[link](<uri>)syntax. The regex could probably be simplified further, but that would be a more complicated change.