-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(eslint-plugin): [restrict-template-expressions] add support for intersection types #1803
feat(eslint-plugin): [restrict-template-expressions] add support for intersection types #1803
Conversation
Thanks for the PR, @ulrichb! 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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
30e03ab
to
88b8d4a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1803 +/- ##
==========================================
- Coverage 94.38% 94.37% -0.01%
==========================================
Files 164 164
Lines 7585 7572 -13
Branches 2180 2175 -5
==========================================
- Hits 7159 7146 -13
Misses 183 183
Partials 243 243
|
cede642
to
3610756
Compare
…intersection types (fixes typescript-eslint#1797)
3610756
to
054e62e
Compare
@bradzacher Also adapted the commit message (fix=>feat) |
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 few comments, otherwise LGTM.
Thanks for your work!
- there was a minor update to this rule in d44c0f9, please update accordingly.
- make sure you merge master in. there were some lint rule changes which will fire on your pr that will need to be addressed
- please also add an example to the rule doc.
} | ||
return rec( | ||
// "Extracts" generic constraint, indexed access and conditional types: | ||
typeChecker.getBaseConstraintOfType(expressionType) ?? expressionType, |
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.
We've got a utility function for this!
typeChecker.getBaseConstraintOfType(expressionType) ?? expressionType, | |
util.getConstrainedTypeAtLocation(typeChecker, expression), |
util.isTypeFlagSet( | ||
type, | ||
ts.TypeFlags.NumberLike | ts.TypeFlags.BigIntLike, | ||
) && | ||
options.allowNumber |
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.
nit - do the option check first to save some cycles
util.isTypeFlagSet( | |
type, | |
ts.TypeFlags.NumberLike | ts.TypeFlags.BigIntLike, | |
) && | |
options.allowNumber | |
options.allowNumber && | |
util.isTypeFlagSet( | |
type, | |
ts.TypeFlags.NumberLike | ts.TypeFlags.BigIntLike, | |
) |
util.isTypeFlagSet(type, ts.TypeFlags.BooleanLike) && | ||
options.allowBoolean |
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.
ditto
util.isTypeFlagSet(type, ts.TypeFlags.BooleanLike) && | |
options.allowBoolean | |
options.allowBoolean && | |
util.isTypeFlagSet(type, ts.TypeFlags.BooleanLike) |
util.isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined) && | ||
options.allowNullable |
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.
ditto
util.isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined) && | |
options.allowNullable | |
options.allowNullable && | |
util.isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined) |
…ssions_intersesion_types # Conflicts: # packages/eslint-plugin/src/rules/restrict-template-expressions.ts
97be444
to
df0ad14
Compare
b82ca11
to
fde1f38
Compare
fde1f38
to
dfe2c64
Compare
|
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.
LGTM - thanks for this
Thanks for reviewing! |
Fixes #1797
@reviewers: I had to refactor the
getBaseType()
function (which extracted all union primitive/"other" types) into a predicate-receivingisUnderlyingExpressionTypeConfirmingTo()
.IMO this is better anyways because it separates the two concerns:
a) "a property x applies to all underlying types", and
b) "some type x is primitive (in respect to the options)"
Also
isUnderlyingExpressionTypeConfirmingTo()
is more generic (easier to re-use).Better ideas for the name "isUnderlyingExpressionTypeConfirmingTo"?