refactor(linter/plugins): add TODO comments for @ts-expect-errors which need fixing#16796
Conversation
github-actions
bot
added
A-linter
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The diff meets the stated goal of adding TODO to relevant @ts-expect-error comments and making formatting more consistent. The main remaining issue is that several TODOs are still vague and may become permanent; making them actionable (e.g., TODO(#issue): ...) would significantly improve maintainability. Consider referencing a single tracked issue for repeated suppressions like “types don’t quite align yet” to avoid TODO drift.
Additional notes (3)
-
Maintainability |
apps/oxlint/src-js/plugins/scope.ts:118-118
Multiple@ts-expect-error - TODO: ...suppressions use the same generic rationale. That’s fine, but it’s still not very actionable; it doesn’t help a future maintainer know whether this is blocked on upstream@typescript-eslint/scope-managertypings, your ESTree alignment/typegen, or something else. Also, since this file has many suppressions with the same message, consider centralizing the mismatch via a typed adapter/wrapper (so you have 1-2 suppressions instead of many), but that may be outside the scope of this PR. -
Maintainability |
apps/oxlint/src-js/plugins/scope.ts:264-264
Not all@ts-expect-errorcomments in this file were converted to TODOs—line 262 remains// @ts-expect-error - Don't want to create a new variable just to make it nullable. Per the PR context (“include TODO where it's an issue we need to fix”), this one is ambiguous: if it’s a deliberate and acceptable suppression, it should not be TODO (fine), but the wording currently mixes the standardized dash format with a non-TODO rationale.
To keep comment conventions consistent, consider either (a) switching to a non-TODO standard format for “intentional” suppressions (if you have one), or (b) rewording this to fit the same pattern (and optionally clarify that it’s intentional/acceptable).
- Maintainability |
apps/oxlint/test/fixtures/parent/plugin.ts:17-18
This is a test fixture and the TODO addition aligns with the PR intent. One concern: the message "We need to fix our types" is vague; even in fixtures, vague TODOs tend to stick around indefinitely. If there’s an existing issue for fixinggetAncestors()typing or theNodedefinition, referencing it would make this TODO meaningfully actionable.
Summary of changes
What changed
- Standardized several
// @ts-expect-errorcomments to include a consistent prefix:// @ts-expect-error - TODO: .... - Updated
@ts-expect-errorannotations across multiple plugin modules (lint,scope,selector,source_code) and a test fixture to clearly flag known type mismatches as TODOs.
Files touched
apps/oxlint/src-js/plugins/lint.tsapps/oxlint/src-js/plugins/scope.tsapps/oxlint/src-js/plugins/selector.tsapps/oxlint/src-js/plugins/source_code.tsapps/oxlint/test/fixtures/parent/plugin.ts
There was a problem hiding this comment.
Pull request overview
This PR standardizes the formatting of @ts-expect-error comments to clearly distinguish type issues that need fixing from intentional suppressions. All comments representing technical debt are updated to use the format // @ts-expect-error - TODO: followed by the explanation.
- Adds "TODO:" to
@ts-expect-errorcomments that represent type issues requiring future fixes - Ensures consistent formatting across all such comments with
- TODO:separator
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/oxlint/test/fixtures/parent/plugin.ts | Updated comment for getAncestors type issue to include TODO |
| apps/oxlint/src-js/plugins/source_code.ts | Updated comments for missing .d.ts file and parent property type issues to include TODO |
| apps/oxlint/src-js/plugins/selector.ts | Updated comment for missing .d.ts file to include TODO |
| apps/oxlint/src-js/plugins/scope.ts | Updated multiple comments for type alignment issues to include TODO consistently |
| apps/oxlint/src-js/plugins/lint.ts | Updated comment for missing .d.ts file to include TODO |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge activity
|
…hich need fixing (#16796) Only alters comments. * Make sure all `// @ts-expect-error` comments include "TODO" where it's an issue we need to fix. * Format all the "TODO" comments consistently.
graphite-app
bot
force-pushed
the
12-13-test_linter_plugins_remove_ts-expect-error_from_tests
branch
from
151fe2b to
26ae895
Compare
graphite-app
bot
force-pushed
the
12-13-refactor_linter_plugins_add_todo_comments_for_ts-expect-error_s_which_need_fixing
branch
from
3b1fbac to
fabe6f7
Compare
graphite-app
bot
deleted the
12-13-refactor_linter_plugins_add_todo_comments_for_ts-expect-error_s_which_need_fixing
branch
1 participant
Only alters comments.
// @ts-expect-errorcomments include "TODO" where it's an issue we need to fix.