feat(eslint-plugin): [prefer-nullish-coalescing] add support for assignment expressions #10152
Conversation
|
Thanks for the PR, @abrahamguo! 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 configuration. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 22841da. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
3dd873d to
1e3281a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10152 +/- ##
==========================================
- Coverage 86.18% 86.16% -0.02%
==========================================
Files 430 430
Lines 15029 15031 +2
Branches 4360 4361 +1
==========================================
- Hits 12952 12951 -1
- Misses 1725 1728 +3
Partials 352 352
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -171,7 +171,107 @@ export default createRule<Options, MessageIds>({ | |||
| }); | |||
| } | |||
|
|
|||
| // todo: rename to something more specific? | |||
There was a problem hiding this comment.
todo
What's the resolution here? FWIW I don't mind the name as-is.
| @@ -197,7 +197,7 @@ function validateConfigSchema( | |||
| config: TesterConfigWithDefaults, | |||
| source: string, | |||
| ): void { | |||
| validateSchema ||= ajv.compile(flatConfigSchema); | |||
| validateSchema ??= ajv.compile(flatConfigSchema); | |||
There was a problem hiding this comment.
[Praise] Nice, a fix in the project itself!
| @@ -30,31 +30,42 @@ const nullishTypes = ['null', 'undefined', 'null | undefined']; | |||
| const ignorablePrimitiveTypes = ['string', 'number', 'boolean', 'bigint']; | |||
|
|
|||
| function typeValidTest( | |||
| cb: (type: string) => ValidTestCase<Options> | string, | |||
| cb: (type: string, equals: '' | '=') => ValidTestCase<Options> | string, | |||
There was a problem hiding this comment.
[Testing] We've been moving away from auto-generated tests. They tend to generate a huge spread of unit tests that each test ever so slightly different variations of the same thing. For example, this one now means each intricacy passed to typeValidTest is tested under both values of equals.
IMO it'd be better to instead have specific targeted tests for each behavior under test.
Change request: revert any additions to these test generation functions, and instead write new tests manually.
|
I'd switched from |
e765033
into
typescript-eslint:main
##### [v8.12.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8120-2024-10-28) ##### 🚀 Features - **eslint-plugin:** \[no-base-to-string] handle String() ([#10005](typescript-eslint/typescript-eslint#10005)) - **eslint-plugin:** \[switch-exhaustiveness-check] add allowDefaultCaseMatchUnionMember option ([#9954](typescript-eslint/typescript-eslint#9954)) - **eslint-plugin:** \[consistent-indexed-object-style] report mapped types ([#10160](typescript-eslint/typescript-eslint#10160)) - **eslint-plugin:** \[prefer-nullish-coalescing] add support for assignment expressions ([#10152](typescript-eslint/typescript-eslint#10152)) ##### ❤️ Thank You - Abraham Guo - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
PR Checklist
Overview
Re-applies #5234, while accounting for updates made since to
prefer-nullish-coalescing.Commits:
LogicalExpressionlistener intocheckAssignmentOrLogicalExpressioncheckAssignmentOrLogicalExpressionto support assignmentsdeclare consts todeclare lets — it is not clear to me why @JoshuaKGoldberg did this in feat(eslint-plugin): [prefer-nullish-coalescing]: add support for assignment expressions #5234, but I have done it