Skip to content
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

Conversation

ulrichb
Copy link
Contributor

@ulrichb ulrichb commented Mar 26, 2020

Fixes #1797

@reviewers: I had to refactor the getBaseType() function (which extracted all union primitive/"other" types) into a predicate-receiving isUnderlyingExpressionTypeConfirmingTo().

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"?

@typescript-eslint
Copy link
Contributor

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.

@ulrichb ulrichb force-pushed the feat/restrict-template-expressions_intersesion_types branch 2 times, most recently from 30e03ab to 88b8d4a Compare March 26, 2020 13:55
@codecov
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #1803 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            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              
Flag Coverage Δ
#unittest 94.37% <100.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
...-plugin/src/rules/restrict-template-expressions.ts 100.00% <100.00%> (ø)

@ulrichb ulrichb force-pushed the feat/restrict-template-expressions_intersesion_types branch 3 times, most recently from cede642 to 3610756 Compare March 26, 2020 14:25
@bradzacher bradzacher added the enhancement New feature or request label Mar 26, 2020
@bradzacher bradzacher changed the title fix(eslint-plugin): [restrict-template-expressions] add support for intersection types feat(eslint-plugin): [restrict-template-expressions] add support for intersection types Mar 26, 2020
@ulrichb ulrichb force-pushed the feat/restrict-template-expressions_intersesion_types branch from 3610756 to 054e62e Compare March 27, 2020 14:45
@ulrichb
Copy link
Contributor Author

ulrichb commented Mar 27, 2020

@bradzacher Also adapted the commit message (fix=>feat)

Copy link
Member

@bradzacher bradzacher left a 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,
Copy link
Member

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!

Suggested change
typeChecker.getBaseConstraintOfType(expressionType) ?? expressionType,
util.getConstrainedTypeAtLocation(typeChecker, expression),

Comment on lines 53 to 57
util.isTypeFlagSet(
type,
ts.TypeFlags.NumberLike | ts.TypeFlags.BigIntLike,
) &&
options.allowNumber
Copy link
Member

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

Suggested change
util.isTypeFlagSet(
type,
ts.TypeFlags.NumberLike | ts.TypeFlags.BigIntLike,
) &&
options.allowNumber
options.allowNumber &&
util.isTypeFlagSet(
type,
ts.TypeFlags.NumberLike | ts.TypeFlags.BigIntLike,
)

Comment on lines 63 to 64
util.isTypeFlagSet(type, ts.TypeFlags.BooleanLike) &&
options.allowBoolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Suggested change
util.isTypeFlagSet(type, ts.TypeFlags.BooleanLike) &&
options.allowBoolean
options.allowBoolean &&
util.isTypeFlagSet(type, ts.TypeFlags.BooleanLike)

Comment on lines 70 to 71
util.isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined) &&
options.allowNullable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Suggested change
util.isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined) &&
options.allowNullable
options.allowNullable &&
util.isTypeFlagSet(type, ts.TypeFlags.Null | ts.TypeFlags.Undefined)

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Apr 12, 2020
…ssions_intersesion_types

# Conflicts:
#	packages/eslint-plugin/src/rules/restrict-template-expressions.ts
@ulrichb ulrichb force-pushed the feat/restrict-template-expressions_intersesion_types branch from 97be444 to df0ad14 Compare April 14, 2020 14:33
@ulrichb ulrichb force-pushed the feat/restrict-template-expressions_intersesion_types branch from b82ca11 to fde1f38 Compare April 14, 2020 14:58
@ulrichb ulrichb force-pushed the feat/restrict-template-expressions_intersesion_types branch from fde1f38 to dfe2c64 Compare April 14, 2020 15:08
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 14, 2020
@ulrichb
Copy link
Contributor Author

ulrichb commented Apr 14, 2020

@bradzacher

  • Merged master and added "allowAny" from d44c0f9
  • Refactor to util.getConstrainedTypeAtLocation()
  • Move the option-checks before the isTypeFlagSet()-checks
  • Add example: stringWithKind: string & { kind?: 'MyString' }

Copy link
Member

@bradzacher bradzacher left a 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

@ulrichb
Copy link
Contributor Author

ulrichb commented Apr 14, 2020

Thanks for reviewing!

@bradzacher bradzacher merged commit cc70e4f into typescript-eslint:master Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[restrict-template-expressions] Missing handling of intersection type
2 participants