Allow underscores in integers with bases #190
Conversation
|
Thank you very much for your contribution. This sounds like a good idea, yes 👍 Can we please add some tests? I think we should disallow things like |
|
Hey @sharkdp, it's pretty much done, I added a lot of tests for every possible case I could think of. |
There was a problem hiding this comment.
Thank you for the updates. I have just a few more comments.
To ease the addition and maintenance of new tests, I would like to introduce insta to the codebase, are you okay with this change?
I am okay with this, but then we should probably go all in and also replace the other tokenizer and parser unit tests with insta (not in this PR)? For the parser unit tests, this could actually be really nice and replace a lot of hand-coded ASTs. We might also be able to get rid of my custom-made ReplaceSpans/replace_spans logic by using instas redactions feature on the span fields — which would be really nice.
|
Hey @sharkdp! On my computer the execution of your test suites goes from 23s to 2s so I guess it's a success somehow. |
Co-authored-by: David Peter <sharkdp@users.noreply.github.com>
irevoire
force-pushed
the
handle-underscore-in-integer-with-base
branch
from
0a60f80 to
7c1df9f
Compare
|
Rebased on main |
There was a problem hiding this comment.
Thank you very much!
On my computer the execution of your test suites goes from 23s to 2s so I guess it's a success somehow.
Awesome!
But I’m really not sure if I did the right thing, please take a look at 0a60f80b.
I had to change some of your type to requires that they're Sync + I derived Clone on a lot of type without knowing if it was actually correct.
Looks good to me 👍
|
Changes are online on https://numbat.dev |
Hey, it would be nice to allow underscores in numbers with bases.