-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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(ast-spec): make BaseNode
& BaseToken
more type-safe
#3560
Conversation
Thanks for the PR, @MichaelDeBoey! 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. |
Nx Cloud ReportCI ran the following commands for commit 77a0f00. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch Sent with 💌 from NxCloud. |
acf0a25
to
4fc73f9
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.
thanks!
@@ -248,7 +248,9 @@ interface RuleContext< | |||
|
|||
// This isn't the correct signature, but it makes it easier to do custom unions within reusable listeners | |||
// never will break someone's code unless they specifically type the function argument | |||
type RuleFunction<T extends TSESTree.BaseNode = never> = (node: T) => void; | |||
type RuleFunction<T extends TSESTree.Node | TSESTree.Token = never> = ( |
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.
I missed this before - it's not possible for you to create a selector on a token - so we don't need the TSESTree.Token
type here.
Also it's been a while - but I think that when I defined this I didn't use the TSESTree.Node
type for two reasons: (1) because it means if you've got some custom AST nodes from another parser - you can't use them, and (2) IIRC there was some perf issues with TS due to the massive size of the Node
union.
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.
I'll export TokenBase
and do TSESTree.NodeBase | TSESTree.TokenBase
instead
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.
Or should I do it with NodeAndTokenData
instead?
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 don't need the token type at all because tokens aren't valid - so you can keep it as just BaseNode
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 need Token
as we're also having a RuleListener
for Comment
& Token
Comment?: RuleFunction<TSESTree.Comment>; |
Token?: RuleFunction<TSESTree.Token>; |
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.
Weird. I don't think either of those actually work.
Maybe they did at some point - but yeah a quick test doesn't look like they work any more.
Maybe - i only quickly tested against astexplorer's version of ESLint (v4).. so worth a quick test against ESLint v7 to confirm
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.
So I should delete these 2 then?
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.
Commented them out (for now) and nothing seems to break
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.
@bradzacher Should I just remove them?
Or what do you think is the best approach now?
398110a
to
8933591
Compare
Codecov Report
@@ Coverage Diff @@
## master #3560 +/- ##
=======================================
Coverage 92.63% 92.64%
=======================================
Files 325 327 +2
Lines 11327 11334 +7
Branches 3195 3195
=======================================
+ Hits 10493 10500 +7
Misses 370 370
Partials 464 464
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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!
Make sure
BaseNode
'stype
is of typeAST_NODE_TYPES
andBaseToken
'stype
is of typeAST_TOKEN_TYPES