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.

Sign up for GitHub

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

Jump to bottom

Parse units on numeric literals and keep them in the AST #5061

Merged
merged 4 commits into from
Jan 21, 2025
Merged

Parse units on numeric literals and keep them in the AST #5061

merged 4 commits into from
Jan 21, 2025

Conversation

nrc
Copy link
Contributor

@nrc nrc commented Jan 14, 2025 •
edited
Loading

We previously tokenise unit suffixes on numeric literals, this PR threads them through parsing and into the AST which then causes a huge amount of breakage in the frontend :-(

@qa-wolf QA Wolf
Copy link

qa-wolf bot commented Jan 14, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@vercel Vercel
Copy link

vercel bot commented Jan 14, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Jan 21, 2025 6:21pm

@nrc nrc force-pushed the nrc-units-parsing branch from 613ad23 to 232e211 Compare January 14, 2025 20:49
@nrc nrc force-pushed the nrc-units-parsing branch from 39d1b6a to 24a456c Compare January 14, 2025 22:38
@nrc nrc force-pushed the nrc-units-parsing branch from 24a456c to 98f608d Compare January 15, 2025 23:35
@codecov Codecov
Copy link

codecov bot commented Jan 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.18182% with 13 lines in your changes missing coverage. Please review.

Project coverage is 86.10%. Comparing base (9a537da) to head (8cc7797).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/wasm-lib/kcl/src/parsing/token/mod.rs 35.71% 9 Missing ⚠️
...asm-lib/kcl/src/parsing/ast/types/literal_value.rs 82.35% 3 Missing ⚠️
src/wasm-lib/kcl/src/parsing/ast/digest.rs 91.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5061      +/-   ##
==========================================
+ Coverage   86.09%   86.10%   +0.01%     
==========================================
  Files          89       89              
  Lines       32389    32445      +56     
==========================================
+ Hits        27884    27938      +54     
- Misses       4505     4507       +2
Flag Coverage Δ
wasm-lib 86.10% <88.18%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nrc nrc force-pushed the nrc-units-parsing branch from 98f608d to e5eb211 Compare January 16, 2025 19:33
@nrc nrc requested a review from jtran January 16, 2025 19:39
@nrc nrc force-pushed the nrc-units-parsing branch from e5eb211 to f6ecd29 Compare January 16, 2025 19:51
@jtran
Copy link
Collaborator

jtran commented Jan 17, 2025

Can you rebase and regenerate simulation test outputs? My artifact graph PR was merged.

nrc added 3 commits January 21, 2025 07:24
@nrc
Code changes
96bcacf 
Signed-off-by: Nick Cameron <[email protected]>
@nrc
test changes
e0ef664 
Signed-off-by: Nick Cameron <[email protected]>
@nrc
Frontend changes
5cdd58e 
Signed-off-by: Nick Cameron <[email protected]>
@nrc nrc force-pushed the nrc-units-parsing branch from f6ecd29 to 5cdd58e Compare January 20, 2025 18:40
@nrc
Copy link
Contributor Author

nrc commented Jan 20, 2025

Can you rebase and regenerate simulation test outputs? My artifact graph PR was merged.

Done (and passing tests)

jtran
jtran approved these changes Jan 21, 2025
View reviewed changes
Copy link
Collaborator

@jtran jtran left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We're still not using the units from literals, correct?

Screenshot 2025-01-21 at 11 27 07 AM

@nrc @jtran
Refactor asNum
8cc7797 
Co-authored-by: Jonathan Tran <[email protected]>
@nrc
Copy link
Contributor Author

nrc commented Jan 21, 2025

We're still not using the units from literals, correct?
Screenshot 2025-01-21 at 11 27 07 AM

Correct

@nrc nrc merged commit 965cb18 into main Jan 21, 2025
33 checks passed
@nrc nrc deleted the nrc-units-parsing branch January 21, 2025 19:29
TomPridham pushed a commit to TomPridham/modeling-app that referenced this pull request Feb 13, 2025
@nrc @jtran @TomPridham
Parse units on numeric literals and keep them in the AST (KittyCAD#5061)
7c181a5 
* Code changes

Signed-off-by: Nick Cameron <[email protected]>

* test changes

Signed-off-by: Nick Cameron <[email protected]>

* Frontend changes

Signed-off-by: Nick Cameron <[email protected]>

* Refactor asNum

Co-authored-by: Jonathan Tran <[email protected]>

---------

Signed-off-by: Nick Cameron <[email protected]>
Co-authored-by: Jonathan Tran <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtran jtran jtran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nrc @jtran