-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Remove the startpos
field from tokens
#55
base: main
Are you sure you want to change the base?
Conversation
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.
looks good to me !
K"\"" => "\"" , | ||
K"Whitespace" => " " , | ||
K"EndMarker" => "" , | ||
]) |
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.
This really looks a lot nicer 👍
I guess the only downside of this is we don't get so much useful line information when a test fails as the @test
macro occurs inside check_lexing()
. A possible alternative could be to rearrange like this
@test lex(str) == [
K"Whitespace" => " " ,
K"Identifier" => "str" ,
...
]
By the way I have a separate token type in |
I'll drop the last commit for now and put that in a separate PR since it does make untokenization a bit more awkward and perhaps that is not worth it. |
Sure. I'm also fine with it as-is though. |
I quite like what you did with the tests, overall. |
this makes untokenization slightly more annoying
52bc042
to
7d3c967
Compare
startpos
field from tokens
The issue as you say is worse line info. Even putting the |
Yeah this is kinda annoying. It's why I went with the verbose style originally. It's really a problem with |
Although... in current Julia the table would have to be a |
Could maybe use JuliaLang/julia#46138 |
well yeah if stdlib versioning wasn't tied to Julia it might be useful :-/ |
This makes "untokenization" slightly more annoying (since you need to look at the previous token to know where to start) but I think it is worth it. Some tests needed to update based on that. It is a bit unclear if this is worth the effort.