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 token span info, fixes #1 and #13 #15

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

Specy
Copy link
Collaborator

@Specy Specy commented Oct 1, 2024

No description provided.

@Specy
Copy link
Collaborator Author

Specy commented Oct 1, 2024

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:

assertion `left == right` failed
  left: "conflict at state \u{1b}[32m5\u{1b}[0m on \u{1b}[32m'a'\u{1b}[0m"
 right: "conflict at state 5 on 'a'"

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 ParsingError, so i guess there will be failing CI tests because of that.

What should i do?

@Specy
Copy link
Collaborator Author

Specy commented Oct 1, 2024

Ok i managed to fix the errors from the CI reports

@umut-sahin
Copy link
Owner

Sorry, it has been a busy week. I'll review it in this weekend, thanks!

@Specy
Copy link
Collaborator Author

Specy commented Oct 9, 2024

Sure

Copy link
Owner

@umut-sahin umut-sahin left a 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!

src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
src/utils.rs Outdated Show resolved Hide resolved
tests/parsing.rs Outdated Show resolved Hide resolved
tests/parsing.rs Outdated Show resolved Hide resolved
src/parser.rs Outdated Show resolved Hide resolved
src/parser.rs Outdated Show resolved Hide resolved
src/parser.rs Outdated Show resolved Hide resolved
@Specy Specy force-pushed the main branch 2 times, most recently from 24dab13 to 3173a5f Compare October 12, 2024 12:06
@Specy
Copy link
Collaborator Author

Specy commented Oct 12, 2024

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

@Specy Specy requested a review from umut-sahin October 13, 2024 11:05
Copy link
Owner

@umut-sahin umut-sahin left a 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?

bindings/typescript/package.json Outdated Show resolved Hide resolved
bindings/typescript/src/utils.ts Outdated Show resolved Hide resolved
bindings/typescript/src/utils.ts Outdated Show resolved Hide resolved
examples/calculator.rs Outdated Show resolved Hide resolved
src/grammar.rs Outdated Show resolved Hide resolved
src/span.rs Outdated Show resolved Hide resolved
src/span.rs Show resolved Hide resolved
src/span.rs Show resolved Hide resolved
src/span.rs Show resolved Hide resolved
src/span.rs Show resolved Hide resolved
@Specy Specy force-pushed the main branch 2 times, most recently from eb91b69 to 3a1a2a4 Compare October 14, 2024 18:24
@Specy Specy requested a review from umut-sahin October 14, 2024 18:26
@Specy
Copy link
Collaborator Author

Specy commented Oct 14, 2024

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

@Specy Specy force-pushed the main branch 2 times, most recently from 6fde81a to ffc3cad Compare October 21, 2024 18:53
src/lib.rs Outdated
Comment on lines 49 to 52
utils::{
count_col_position,
count_new_lines,
},
Copy link
Owner

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.

@umut-sahin umut-sahin merged commit cc16611 into umut-sahin:main Nov 7, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants