Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Updated typescript-eslint to v6 #54693
Updated typescript-eslint to v6 #54693
Changes from 10 commits
a6f12dd
b001345
4951fc2
f810a22
70caa0c
052f6ed
ee035f7
0fde8d4
170c4e8
2b56c5a
880768e
53e2ae1
0b72601
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this off because the tsconfig has the equivalent Typescript setting on? If not, what kind and how many unused variables do we have?
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.
Yes, it's this; honestly I would much prefer if we didn't use TS for this at all and instead made it a lint. So annoying to have to ensure my variables are all used just to run tests or something. At least debug skips typecheck.
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.
Only 18 failures at the moment. Not bad!
However, typescript-eslint/typescript-eslint#5017 -> gajus/eslint-plugin-jsdoc#858 is impacting the codebase here. Probably best to set up the workarounds in a separate PR IMO.
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.
Is this off because it causes lots of churn? If so, I'd like to see it turned on, but maybe 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.
Yeah, and also because the rule has more false positives right now than I'm comfortable with - especially in a codebase like TypeScript's that does a lot of funky stuff with scoping.
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 honestly shocked we aren't getting lints from
prefer-nullish-coalescing
too... I was restructuring the lint config ofhereby
and hit some false positives there too on code like:Where the actual intent is that the empty string goes away. But maybe that's not a false positive.