feat(eslint-plugin): [restrict-template-expressions] add allowArray option
#8389
Conversation
…estrict-template-expressions
|
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. |
allowArray option for restrict-template-expressionsallowArray option for restrict-template-expressions
…estrict-template-expressions
allowArray option for restrict-template-expressionsallowArray option for restrict-template-expressions
…estrict-template-expressions
…estrict-template-expressions
…estrict-template-expressions
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8389 +/- ##
==========================================
- Coverage 87.21% 87.20% -0.01%
==========================================
Files 251 251
Lines 12315 12308 -7
Branches 3883 3875 -8
==========================================
- Hits 10740 10733 -7
Misses 1305 1305
Partials 270 270
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
This is a clever way of automating the repeated parts 🙂. Nicely done.
...but like many clever code things, there was a difficulty cost of reading it at first. I played around with it locally and found it easier to read through with a few small structural changes. What do you think?
packages/eslint-plugin/src/rules/restrict-template-expressions.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/restrict-template-expressions.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/restrict-template-expressions.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/restrict-template-expressions.ts
Outdated
Show resolved
Hide resolved
…estrict-template-expressions
…estrict-template-expressions
…estrict-template-expressions
…estrict-template-expressions
There was a problem hiding this comment.
Super, looks good to me! Thanks for all the code tinkering. 😄
Leaving open with 1 approval as I know at least @bradzacher has had well-informed thoughts on this family of rules in the past.
…estrict-template-expressions
…estrict-template-expressions
…estrict-template-expressions
allowArray option for restrict-template-expressionsallowArray option
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| recursivelyCheckType(type.getNumberIndexType()!), | ||
| ], | ||
| // eslint-disable-next-line @typescript-eslint/internal/prefer-ast-types-enum |
There was a problem hiding this comment.
@JoshuaKGoldberg - Originally this rule's purpose was to prevent code like node.type === 'string'.
Given we now have the no-unsafe-enum-comparison rule - do you think this internal rule still has value? WDYT about deleting it? (we already have nuec turned on via the recommended config)
There was a problem hiding this comment.
Yeah I'd be in favor of that. It wouldn't be the first example of an internal lint rule ending up as a subset of a general good one.
PR Checklist
Overview
I've added a new
allowArrayoption that applies the existing checks fromrestrict-template-expressionsrecursively (i.e. noundefineds, etc.).I haven't added unit tests yet but I'd like to begin receiving feedback on this change.