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

Allow underscores in number literals #20

Merged
merged 5 commits into from
Apr 7, 2022

Conversation

wackbyte
Copy link
Contributor

@wackbyte wackbyte commented Apr 6, 2022

Allows basically the same things as Rust.
The underscores are removed during codegen.
Closes #7.

@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #20 (23a47fb) into main (61f026e) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
+ Coverage   76.76%   76.79%   +0.03%     
==========================================
  Files         105      105              
  Lines        6076     6086      +10     
==========================================
+ Hits         4664     4674      +10     
  Misses       1412     1412              
Impacted Files Coverage Δ
crates/ditto-checker/src/typechecker/pre_ast.rs 100.00% <100.00%> (ø)
...rates/ditto-checker/src/typechecker/tests/float.rs 100.00% <100.00%> (ø)
crates/ditto-checker/src/typechecker/tests/int.rs 100.00% <100.00%> (ø)
crates/ditto-codegen-js/src/convert.rs 97.33% <100.00%> (ø)
crates/ditto-cst/src/parser/expression.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61f026e...23a47fb. Read the comment docs.

Copy link
Member

@jmackie jmackie left a comment

Choose a reason for hiding this comment

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

Woah thanks so much @wackbyte, I really wasn't expecting any contributions this soon ❤️

Interesting that you removed the underscores during the JavaScript codegen. My first thought was to remove them when converting from CST to AST - that way the removal can happen in one place rather than in all the code generators (there will eventually be more than one...). But then again, maybe it makes sense to handle it on a per-codegen basis?

Not a big deal at all, happy to merge this PR as it is, but would be keen to hear your thoughts!

@wackbyte
Copy link
Contributor Author

wackbyte commented Apr 6, 2022

Doing it during AST conversion does make more sense, actually. I think I tried to do it but wasn't able to find where it was in the code 😅

@jmackie
Copy link
Member

jmackie commented Apr 6, 2022

Doing it during AST conversion does make more sense, actually. I think I tried to do it but wasn't able to find where it was in the code 😅

Ahh that's my bad. The conversion from CST to an intermediate/pre-AST happens here, then there's another conversion (as part of type-checking) that happens here.

I'm happy to make the change if you want? Let me know 🙂

@jmackie jmackie merged commit 45b5504 into ditto-lang:main Apr 7, 2022
@jmackie jmackie mentioned this pull request Apr 7, 2022
4 tasks
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.

Allow underscores in number literals
2 participants