Repository navigation
fix(visitor-keys): reorder TSSatisfiesExpression and TSTypeAssertion - #10139
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. |
| TSQualifiedName: ['left', 'right'], | ||
| TSReadonlyKeyword: [], | ||
| TSRestType: ['typeAnnotation'], | ||
| TSSatisfiesExpression: [ |
There was a problem hiding this comment.
Just noting for the record that it looks like this was set up this way from the start in #5717. The erroneous/misleading comment was added in #5926 with no particular comment. There isn't any discussion of the visitor key order in #5688 either.
Just saying this all to do due diligence to investigate whether there was any particular reason in the paper trail that it was set up that way in the first place, but it doesn't seem there is (see @bradzacher's comment eslint/eslint#19002 (comment))
There was a problem hiding this comment.
I don't remember why I did this.
It was probably a cock up and I copied TSTypeAssertion rather than TSAsExpression.
If no tests fail then I assume it was defs just a bug 🤷
| TSTypeAliasDeclaration: ['id', 'typeParameters', 'typeAnnotation'], | ||
| TSTypeAnnotation: ['typeAnnotation'], | ||
| TSTypeAssertion: SharedVisitorKeys.TypeAssertion, | ||
| // this is intentionally different to SharedVisitorKeys.AsExpression because |
There was a problem hiding this comment.
Personally I don't think this comment is necessary, and would be better omitted (especially considering the previous incarnation of it was wrong). But it's not important one way or another 🤷♂️
kirkwaiblinger
added
the
1 approval
| TSTupleType: ['elementTypes'], | ||
| TSTypeAliasDeclaration: ['id', 'typeParameters', 'typeAnnotation'], | ||
| TSTypeAnnotation: ['typeAnnotation'], | ||
| TSTypeAssertion: SharedVisitorKeys.TypeAssertion, |
There was a problem hiding this comment.
Also noting for the record that this was previously, apparently unintentionally, flipped in https://github.com/typescript-eslint/typescript-eslint/pull/5926/files#diff-04c8c6315c623a82f3004fbfe093162ab6811b1f207cf1832d495b2979466f5eL139
…isitor-keys-satisfies
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10139 +/- ##
=======================================
Coverage 86.14% 86.14%
=======================================
Files 428 428
Lines 14981 14981
Branches 4344 4344
=======================================
Hits 12905 12905
Misses 1730 1730
Partials 346 346
Flags with carried forward coverage won't be shown. Click here to find out more.
|
github-actions
Bot
removed
the
1 approval
|
Thanks heaps! |
PR Checklist
satisfies#10112Overview