feat: Ignore ESLint directive comments in JavaScript and TypeScript#34
Conversation
|
@dnut just circling back around - what do you think of this PR? |
|
Hey @dnut, is there anything that I can do to help get this reviewed and merged? |
|
@dnut just wanted to check in again, in case you would have time soon to help get this landed. |
|
I think there were a few problems with this implementation. Did you test it? I committed a couple fixes that made it work for me. Let me know what you think. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for preventing rewrapping of ESLint configuration comment directives (eslint-disable-next-line and eslint-disable-line) in JavaScript and TypeScript files. These single-line directives must remain on one line to function properly, as breaking them across multiple lines causes ESLint to fail to recognize them.
Changes:
- Added a new
eslintConfigCommentscontent parser wrapper that detects ESLint directive comments and prevents them from being wrapped - Created a new
javascriptDocumentProcessor that integrates the ESLint parser with existing JavaScript/TypeScript comment handling - Updated JavaScript and TypeScript language definitions to use the new processor
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/Parsers.fs | Added eslintConfigComments parser that detects and prevents wrapping of eslint-disable-next-line and eslint-disable-line directives |
| core/Parsing.Documents.fs | Created javascript DocumentProcessor and updated JavaScript/TypeScript language configs to use it |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@dnut oh thanks! Sorry - I was trying to do that by hand, without much knowledge of F# Upon review, your two comments seem to make sense: As per the comment from Copilot, I also added tests: AI disclosure: I used gpt-5.3-codex and reviewed the output as best I could |
eslint-disable-next-line + eslint-disable-line line comments|
In 22cc3ea, I also added support for |
dnut
left a comment
There was a problem hiding this comment.
Before your changes, there were no test failures:
> ./do test
Core build complete. Running tests:...
Passed: 476; Failed: 0; Errored: 0
I ran the tests on this branch and there are six errors:
> ./do test core
Core build complete. Running tests:...
Error: InvalidWrappingColumn, specs/features/block-comments.md
==Input==
/* eslint-disable no-console, @typescript-eslint/no-base-to-string -- temporary exception for migration script with long explanation that should stay intact on one line */
nextLoad(url).source ¦ -> /* eslint-disable no-console, @typescript-eslint/no-base-to-string -- temporary exception for migration script with long explanation that should stay intact on one line */
nextLoad(url).source nextLoad(url).source ¦
Error: InvalidWrappingColumn, specs/features/block-comments.md
==Input==
/* eslint-enable no-console, @typescript-eslint/no-base-to-string -- restore lint checks after migration script with long explanation that should stay intact on one line */
nextLoad(url).source ¦ -> /* eslint-enable no-console, @typescript-eslint/no-base-to-string -- restore lint checks after migration script with long explanation that should stay intact on one line */
nextLoad(url).source nextLoad(url).source ¦
Error: InvalidWrappingColumn, specs/features/line-comments.md
==Input==
// eslint-disable-next-line @typescript-eslint/no-base-to-string -- ModuleSource returns useful information from .toString()
nextLoad(url).source ¦ -> // eslint-disable-next-line @typescript-eslint/no-base-to-string -- ModuleSource returns useful information from .toString()
nextLoad(url).source nextLoad(url).source ¦
Error: InvalidWrappingColumn, specs/features/line-comments.md
==Input==
// eslint-disable-line @typescript-eslint/no-base-to-string -- ModuleSource returns useful information from .toString()
nextLoad(url).source ¦ -> // eslint-disable-line @typescript-eslint/no-base-to-string -- ModuleSource returns useful information from .toString()
nextLoad(url).source nextLoad(url).source ¦
Error: InvalidWrappingColumn, specs/features/line-comments.md
==Input==
// eslint-disable no-console, @typescript-eslint/no-base-to-string -- temporary exception for migration script
nextLoad(url).source ¦ -> // eslint-disable no-console, @typescript-eslint/no-base-to-string -- temporary exception for migration script
nextLoad(url).source nextLoad(url).source ¦
Error: InvalidWrappingColumn, specs/features/line-comments.md
==Input==
// eslint-enable no-console, @typescript-eslint/no-base-to-string -- restore lint checks after migration script
nextLoad(url).source ¦ -> // eslint-enable no-console, @typescript-eslint/no-base-to-string -- restore lint checks after migration script
nextLoad(url).source nextLoad(url).source ¦
Passed: 476; Failed: 0; Errored: 6
dnut
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
There were still some test failures, so I fixed them. The CI was green, but it isn't working properly because test failures are not resulting in non-zero error codes. I'll fix that in another PR.
Closes #33
Rewrap splits
// eslint-disable-next-lineand// eslint-disable-lineinto multiple lines, which breaks behavior because ESLint no longer applies the directive to the intended line.Other directive forms like
eslint-disableandeslint-enableuse multi-line comments, - still valid after wrapping - but they become harder to read and review.Ignore both forms of ESLint directive comments during wrapping in JavaScript and TypeScript and adds specs for these cases.