fix: no-missing-label-refs should not crash on undefined labels #290
Conversation
| { | ||
| code: "[foo]\n[foo][bar]", | ||
| errors: [ | ||
| { | ||
| messageId: "notFound", | ||
| data: { label: "foo" }, | ||
| line: 1, | ||
| column: 2, | ||
| endLine: 1, | ||
| endColumn: 5, | ||
| }, | ||
| { | ||
| messageId: "notFound", | ||
| data: { label: "bar" }, | ||
| line: 2, | ||
| column: 7, | ||
| endLine: 2, | ||
| endColumn: 10, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
This was another bug: the regex that looks for [foo][bar] matched first and then the loop just skipped the previous code so only one error was reported.
| findOffsets(node.value, columnStart); | ||
|
|
||
| const startLine = nodeStartLine + startLineOffset; | ||
| const startColumn = nodeStartColumn + startColumnOffset; |
There was a problem hiding this comment.
Columns were wrong in some cases, for two reasons:
TextNode#valueis not always the raw text of the node - some lines are trimmed left, and some aren't.findOffsets():columnOffsetis the column offset only when it's on the same line. For other lines,columnOffsetis 0-based column number, not offset.
There was a problem hiding this comment.
Good point on 2. Can you add comments to findOffsets() to document that?
There was a problem hiding this comment.
I added a note on how columnOffset should be used in 63d3b4d.
| if (!Array.isArray(match)) { | ||
| break; | ||
| } | ||
| const labelPattern = /\[(?<left>[^\]]*)\](?:\[(?<right>[^\]]*)\])?/dgu; |
There was a problem hiding this comment.
Can we pull this regex out into the module scope? There doesn't seem to be anything context-sensitive that would require it to be in here. I'd rather parse regexes once vs. every time the function is called.
Also, it would be helpful to add a comment explaining what the regex does and how it is used.
There was a problem hiding this comment.
Also, it would be helpful to add a comment explaining what the regex does and how it is used.
Added in 60d5735.
There was a problem hiding this comment.
Can we pull this regex out into the module scope? There doesn't seem to be anything context-sensitive that would require it to be in here. I'd rather parse regexes once vs. every time the function is called.
The g flag makes it mutable - each .exec() modifies lastIndex. I think it's a good practice to keep those regex instances scoped to the search instance that's using them, especially if they are used in a loop, because a small change in the code, like adding an early exit in the loop, can cause bugs that are difficult to reproduce and figure out.
|
|
||
| while ((match = labelPattern.exec(nodeText))) { |
There was a problem hiding this comment.
Can you add a comment explaining how this loop works?
| findOffsets(node.value, columnStart); | ||
|
|
||
| const startLine = nodeStartLine + startLineOffset; | ||
| const startColumn = nodeStartColumn + startColumnOffset; |
There was a problem hiding this comment.
Good point on 2. Can you add comments to findOffsets() to document that?
Fixes #289