-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(eslint-plugin): [no-extra-parens] false positive when used with satisfies
keyword
#7026
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
Conversation
Thanks for the PR, @fcorsair! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
satisfies
keywordsatisfies
keyword
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.
A good start, thanks for sending this in! 🙌
Requesting changes for increased test coverage. This stuff can get tricky! I'd suggest seeing the discussion in #6885 to check if there's some shared logic that should be updated. I can review more deeply once there are more test cases.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7026 +/- ##
=======================================
Coverage 87.38% 87.38%
=======================================
Files 386 386
Lines 13193 13195 +2
Branches 3867 3868 +1
=======================================
+ Hits 11529 11531 +2
Misses 1298 1298
Partials 366 366
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This comment was marked as spam.
This comment was marked as spam.
@@ -242,6 +242,11 @@ f<(number | string)[]>(['a', 1]) | |||
}, | |||
}, | |||
}), | |||
{ | |||
code: ` | |||
const a = (['a', 'b'] satisfies A[]).includes('a'); |
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.
Recreating a new thread from https://github.com/typescript-eslint/typescript-eslint/pull/7026/files#r1192868544: shouldn't this still cause a rule report?
const a = ((['a', 'b'] satisfies A[])).includes('a');
Note the extra parenthesis around the (['a', 'b'] satisfies A[])
.
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.
sure, this case definitely deserves a test
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.
Sorry if I was unclear on this one - the code looks nice in general, but I think there's a missing test case that fails locally for me?
Ping @fcorsair, is this still something you have time & interest for? |
yes, I would like to complete this PR, I've been a little busy in the last months |
Closing this PR as it's been stale for ~6 weeks without activity. Feel free to reopen @fcorsair if you have time - but no worries if not! If anybody wants to drive it forward, please do post your own PR - and if you use this as a start, consider adding |
Hi @JoshuaKGoldberg I don’t get from the issue discussion if the problem should be now fixed from latest eslint versions or still not. Do you confirm it? |
I'm not sure what you mean, but if there's a bug in typescript-eslint with an equivalent that was fixed in ESLint core we can always take an issue. |
Ok now I get it, it’s fixed in eslint only, still bugged in ts-eslint |
PR Checklist
satisfies
keyword #7017Overview
Added new condition inside
MemberExpression
type rule for SatisfiesExpression node type, added new simple test.This is my first PR, be kind 😄 ❤️