Skip to content
This repository was archived by the owner on Jan 14, 2019. It is now read-only.

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Dec 1, 2018

Add proper typing to convert.ts
this PR contains logical change (it's not affecting resulted ast)

while doing this i found few issues with current implementation and i marked with // TODO for feature investigation:

@armano2 armano2 changed the title refactor: Improve type checking in convert.ts refactor: improve type checking in parser Dec 1, 2018
@armano2 armano2 force-pushed the types-in-convert branch 2 times, most recently from 7736feb to c7af360 Compare December 2, 2018 00:49
@armano2
Copy link
Contributor Author

armano2 commented Dec 3, 2018

@JamesHenry am i going in correct direction?

i started also adding type checking for AST from eslint: @types/estree

@JamesHenry
Copy link
Owner

JamesHenry commented Dec 5, 2018

I only have some nits with this, such as the camelCase file name tsNodes and the exported type AllTSNodes feels less intuitive than the singular TSNode as its usage is more like a enum, e.g. in the type assertion as AllTSNodes, but this is a significant improvement so I won't let the nits block anything.

I'm travelling this week as I mentioned, so only have very limited chance to look at these PRs, and that's why I prioritized the low risk chores/refactors

Thanks again for your support 👍

Copy link
Owner

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Thanks again!

@JamesHenry JamesHenry merged commit f5dcd5d into JamesHenry:master Dec 13, 2018
@armano2 armano2 deleted the types-in-convert branch December 13, 2018 22:25
@armano2
Copy link
Contributor Author

armano2 commented Dec 13, 2018

:>

@JamesHenry
Copy link
Owner

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants