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: add missing node type guards #33

Merged

Conversation

RebeccaStevens
Copy link
Collaborator

@RebeccaStevens RebeccaStevens commented Feb 8, 2023

PR Checklist

Overview

  • Adds the missing node type guards from tsutils
  • Adds node type guards for node introduced to TypeScript since tsutils was last updated
  • Add a bunch of "union node type guards" (node type guards simple made up of unions of other node type guards)
  • BREAKING: isExpression removed

When making this PR, I noticed that it's probably possible to auto-generate a bunch of these type-guards; particularly with the ones defined in simple.ts and union.ts. This is probably worth looking into in future.

Note: No additionally test have been added.

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

This is fantastic, thanks for getting so much of this sent @RebeccaStevens! 🎉

Requesting changes to remove some excess functions. Very excited to see it come in!

.prettierignore Outdated Show resolved Hide resolved
src/nodes/typeGuards/simple.ts Outdated Show resolved Hide resolved
src/nodes/typeGuards/simple.ts Outdated Show resolved Hide resolved
src/nodes/typeGuards/index.ts Outdated Show resolved Hide resolved
src/nodes/typeGuards/index.ts Outdated Show resolved Hide resolved
src/nodes/typeGuards/alias.ts Outdated Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Feb 8, 2023
@RebeccaStevens
Copy link
Collaborator Author

I think this PR is ready other than for knip complaining. I'm not sure why it's complaining.

@JoshuaKGoldberg
Copy link
Owner

...huh, yeah, me neither. Weird. I'm guessing it's an export * that includes export *s? webpro-nl/knip#62

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Very close!

For the Knip issues, pending webpro-nl/knip#62, I guess you can add an explicit ignore to the config file. 😞

src/nodes/typeGuards/index.ts Outdated Show resolved Hide resolved
typings/typescript.d.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@RebeccaStevens RebeccaStevens mentioned this pull request Feb 9, 2023
3 tasks
@RebeccaStevens
Copy link
Collaborator Author

I guess you can add an explicit ignore to the config file

This isn't as easy as you would think it would be.
ignore seems to be "ignore these files entirely", not just "ignore any issue from these files".
So ignore all the files in src/nodes/typeGuards, then there is a cascading effect where createNode is now unused. If I ignore that then the dependency @typescript/vfs is now unused.

seems that knip has a bug and where these falsely report issues
@RebeccaStevens
Copy link
Collaborator Author

RebeccaStevens commented Feb 9, 2023

This isn't as easy as you would think it would be. ignore seems to be "ignore these files entirely", not just "ignore any issue from these files". So ignore all the files in src/nodes/typeGuards, then there is a cascading effect where createNode is now unused. If I ignore that then the dependency @typescript/vfs is now unused.

Ignoring only the non-test files actually seems to resolve this.

Those it's fragile as the files are ignored completely, not just issues ignored.

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

This is fantastic, thanks a million @RebeccaStevens! 🚀 Really appreciate you iterating with me & doing such a thorough job.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 8e46284 into JoshuaKGoldberg:main Feb 10, 2023
@RebeccaStevens RebeccaStevens deleted the node-type-guards-2 branch February 13, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Needs an action taken by the original poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Remove isTextualLiteral 🐛 Bug: isExpression probably doesn't match all expressions
2 participants