-
Notifications
You must be signed in to change notification settings - Fork 6
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 token span info, fixes #1 and #13 #15
Conversation
Also one info, tests on my machine are not executing successfully because of the colored strings not being equal to the normal string slices, for example:
I've changed two tests to make them not use error messages, but i wasn't sure if you wanted them to stay that way so i've left them. I added token span info in What should i do? |
Ok i managed to fix the errors from the CI reports |
Sorry, it has been a busy week. I'll review it in this weekend, thanks! |
Sure |
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.
Let's move span related types and impls to their own module.
Create src/span.rs
and move related code there please.
I'll give another review once the PR is updated. Thanks!
24dab13
to
3173a5f
Compare
I forgot to change the local package.json version back to v0.3.0, be careful when publishing, make sure to change it back to v0.3.1 |
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 to test whether the computed spans are correct. Could you add/update tests to make sure spans are calculated correctly in tests/parsing.rs
?
eb91b69
to
3a1a2a4
Compare
I have not added tests for the typescript lib because i'm getting error with the node environment running with wasm and it's a bit of a mess (love js sometimes) so i've left it for a future PR, but i've added the tokenization span test, only one but it covers many cases of where it could be buggy |
6fde81a
to
ffc3cad
Compare
src/lib.rs
Outdated
utils::{ | ||
count_col_position, | ||
count_new_lines, | ||
}, |
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.
Let's not re-export these since they are for internal use.
No description provided.