-
-
Couldn't load subscription status.
- Fork 705
feat(linter/plugins): comment-related APIs #14715
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(linter/plugins): comment-related APIs #14715
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
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.
Pull Request Overview
Introduce and test comment-related SourceCode APIs for JS plugins, replacing the previous single getAllComments fixture with a more comprehensive comments fixture.
- Implement SourceCode.getCommentsBefore, getCommentsAfter, getCommentsInside, and commentsExistBetween
- Add new test fixture “comments” with snapshot; remove old “getAllComments” fixture
- Update e2e test to use the new fixture
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/oxlint/src-js/plugins/source_code.ts | Implements the new comment-related APIs used by plugins. |
| apps/oxlint/test/e2e.test.ts | Points the e2e test to the new “comments” fixture (currently marked it.only). |
| apps/oxlint/test/fixtures/comments/plugin.ts | New test plugin exercising the new comment-related APIs. |
| apps/oxlint/test/fixtures/comments/output.snap.md | Snapshot for the new comments fixture. |
| apps/oxlint/test/fixtures/comments/.oxlintrc.json | Config enabling the new test plugin for the fixture. |
| apps/oxlint/test/fixtures/getAllComments/* | Removes the old fixture, plugin, and snapshot tied to getAllComments. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
3b6e594 to
5b96e1f
Compare
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.
Apologies that this review is premature - I know you're still working on it. But I was interested and had a look, so I thought I'd give my thoughts.
Background: In linter JS plugins, the JS-side code is going to be the slowest code in the whole path, so I'm keen to get it as tight and performant as it possibly can be. In most projects, readability and suchlike is also important, and it's normal to be a little loose, but here I think we should prioritise perf over all else.
(e.g. you'll see in the rest of the JS plugins code we never use if (!foo), we'll use the cheaper if (foo === false) or if (foo !== null) instead, depending on the type of foo)
That's the reason for my rather fanatical nit-picking!
Note: I think it's fine to leave binary search to a later PR, but I still feel it's helpful to tighten the algorithms as much as possible at this earlier stage.
9d4fa3f to
60f0b14
Compare
|
Great! Reviewing now (rebasing on latest main first). |
60f0b14 to
5fc0f23
Compare
|
@lilnasy I'm really sorry, I've made a massive screw-up. While rebasing on main, it seems my local git client had cached the previous version (before you added the whitespace-checking logic). Now I've force-pushed and destroyed all that work. Could you please force push the most recent version from your local copy, to restore the most recent version? I'm really sorry. Totally stupid of me! |
5fc0f23 to
2a563b5
Compare
|
OK, I think I've recovered the commits I over-wrote, and force pushed them back onto the PR branch. But can you please check I didn't lose anything in the process? Really sorry I've made a mess of this. |
|
I'm still going through it. Have pushed a few commits - all just nits, except the last one. |
Destructuring treats array as an iterator. Direct property access may be faster.
If there's no comments after `betweenRangeStart`, `betweenRangeEnd` is never accessed. So move it to where it's used.
471dfc3 to
1aadc6d
Compare
Use comments instead for clarity.
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 one! I had completely misunderstood what these methods are meant to do, because I hadn't looked at ESLint's implementation. Thank you for figuring out the correct logic.
I've pushed a bunch of commits, but they're all just style nits, perf micro-micro-optimizations, and increasing the test coverage. The logic is unaltered, and all seems correct.
I've not checked the output against ESLint, but I have been through the snapshot and checked it's correct according to my new understanding of what these methods are meant to do.
The only one of my commits which actually changed behavior is this one: 1aadc6d
Going to merge now, but please come back if you disagree with any of my changes.
Follow-on after #14715. I for one had completely misunderstood what the comments APIs are meant to do. Expand the docs for these methods to clarify their behavior.
Part of #14564. Implement the remaining
SourceCodeAPIs related to comments (getCommentsBefore,getCommentsAfter,getCommentsInside,commentsExistBetween).