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: merge AST structure of TSAsExpression with TSTypeAssertion #3005

Closed
wants to merge 1 commit into from

Conversation

armano2
Copy link
Member

@armano2 armano2 commented Feb 6, 2021

This PR aims to merge AST structure of TSAsExpression with TSTypeAssertion

This is BREAKING CHANGE

New AST structure

interface TSTypeAssertion extends Node {
  type: AST_NODE_TYPES.TSTypeAssertion;
  typeAnnotation: TypeNode;
  expression: Expression;
  kind: 'as' | 'angle-bracket';
}

TODO:

  • Add unit tests for indent rule
  • Add unit tests for keyword-spacing rule
  • Add unit tests for no-var-requires rule

fixes #2142

@armano2 armano2 added breaking change This change will require a new major version to be released AST PRs and Issues about the AST structure labels Feb 6, 2021
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @armano2!

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.

@armano2 armano2 added the DO NOT MERGE PRs which should not be merged yet label Feb 6, 2021
@armano2 armano2 changed the title feat: merge ast structure of as expression with type assertion feat: merge AST structure of as expression with type assertion Feb 6, 2021
@armano2 armano2 changed the title feat: merge AST structure of as expression with type assertion feat: merge AST structure of TSAsExpression with TSTypeAssertion Feb 6, 2021
@armano2 armano2 added enhancement New feature or request and removed DO NOT MERGE PRs which should not be merged yet labels Feb 6, 2021
Comment on lines +698 to +704
if (node.kind === 'as') {
this.visit(node.expression);
this.visitType(node.typeAnnotation);
} else {
this.visitType(node.typeAnnotation);
this.visit(node.expression);
}
Copy link
Member Author

@armano2 armano2 Feb 6, 2021

Choose a reason for hiding this comment

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

while doing this change i noticed that order of creating references was not correct

@codecov
Copy link

codecov bot commented Feb 6, 2021

Codecov Report

Merging #3005 (16a89b1) into master (5e2a993) will decrease coverage by 0.01%.
The diff coverage is 76.47%.

@@            Coverage Diff             @@
##           master    #3005      +/-   ##
==========================================
- Coverage   92.87%   92.85%   -0.02%     
==========================================
  Files         314      314              
  Lines       10670    10671       +1     
  Branches     3026     3029       +3     
==========================================
- Hits         9910     9909       -1     
  Misses        344      344              
- Partials      416      418       +2     
Flag Coverage Δ
unittest 92.85% <76.47%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...nt-plugin/src/rules/indent-new-do-not-use/index.ts 98.13% <ø> (ø)
...ackages/eslint-plugin/src/rules/keyword-spacing.ts 81.25% <0.00%> (-11.61%) ⬇️
...ackages/eslint-plugin/src/rules/no-var-requires.ts 88.88% <ø> (ø)
...ackages/eslint-plugin/src/rules/prefer-as-const.ts 100.00% <ø> (ø)
packages/eslint-plugin/src/rules/unbound-method.ts 93.10% <ø> (ø)
...ges/experimental-utils/src/ast-utils/predicates.ts 37.50% <0.00%> (+0.65%) ⬆️
...pt-estree/src/ts-estree/estree-to-ts-node-types.ts 100.00% <ø> (ø)
packages/visitor-keys/src/visitor-keys.ts 100.00% <ø> (ø)
packages/eslint-plugin/src/rules/indent.ts 89.13% <80.00%> (-1.35%) ⬇️
...int-plugin/src/rules/consistent-type-assertions.ts 84.84% <100.00%> (-1.64%) ⬇️
... and 4 more

Comment on lines +180 to +181
node.type === AST_NODE_TYPES.TSTypeAssertion &&
node.kind === 'as'
Copy link
Member Author

Choose a reason for hiding this comment

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

there is no unit case for type assertion with brackets

@armano2 armano2 added this to the 5.0.0 milestone Feb 19, 2021
@armano2
Copy link
Member Author

armano2 commented Feb 24, 2021

i implemented this change but I'm unsure if this is actually good

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.

i implemented this change but I'm unsure if this is actually good

What do you mean?
Do you mean you don't like having one node represent the two different types?

Comment on lines +34 to +37
TSTypeAssertion(node): void {
if (node.kind !== 'as') {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

could do this via a selector

Suggested change
TSTypeAssertion(node): void {
if (node.kind !== 'as') {
return;
}
'TSTypeAssertion[kind = "as"]'(node): void {

Copy link
Member

Choose a reason for hiding this comment

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

we could also add this as a typed selector to packages/types/src/ast-node-types.ts as it's probably going to be a more common usecase

@armano2
Copy link
Member Author

armano2 commented Feb 24, 2021

What do you mean?
Do you mean you don't like having one node represent the two different types?

yes, I'm not sure if this is a good change, for sure its more convenient

@bradzacher bradzacher removed this from the 5.0.0 milestone Aug 21, 2021
@bradzacher bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Mar 4, 2022
@bradzacher bradzacher closed this Mar 4, 2022
@bradzacher bradzacher deleted the feat/remove-ts-as-expression branch March 4, 2022 07:40
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
AST PRs and Issues about the AST structure breaking change This change will require a new major version to be released enhancement New feature or request stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] unify type assertion nodes
2 participants