Remove KclValue::SketchGroup variant #3446

adamchalmers · 2024-08-15T03:20:36Z

We can store Rust types like SketchGroup as their own variant of KclValue, or as KclValue::UserVal. Sometimes we store in one and try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using the other. If we stopped using ::UserVal, we'd need a new variant for every Rust type we wanted to build, including user-defined types. So I don't think that's practical.

Instead, we should store every KCL value by de/serializing it into UserVal. This is a first step along that path, removing just the SketchGroup variants. If it goes well, we can remove the other specialized variants too.

My only concern is there might be performance implications from how frequently we convert between serde_json::Value and Rust types via Serde. But I'm not too worried -- there's no parsing JSON strings, just traversing serde_json::Value trees. This isn't great for performance but I think it'll probably be miniscule in comparison to doing all the API calls. I'll benchmark it and see.

qa-wolf · 2024-08-15T03:20:40Z

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 · 2024-08-15T03:20:41Z

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

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Aug 21, 2024 3:57pm

jessfraz

nice im stoked

codecov · 2024-08-15T04:01:32Z

Codecov Report

Attention: Patch coverage is 96.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 87.92%. Comparing base (682590d) to head (50af13b).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/wasm-lib/kcl/src/executor.rs	92.85%	6 Missing ⚠️
src/wasm-lib/kcl/src/std/args.rs	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3446      +/-   ##
==========================================
+ Coverage   87.83%   87.92%   +0.08%     
==========================================
  Files          66       66              
  Lines       25963    26003      +40     
==========================================
+ Hits        22805    22863      +58     
+ Misses       3158     3140      -18

Flag	Coverage Δ
wasm-lib	`87.92% <96.50%> (+0.08%)`	⬆️

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.

jtran

What you said about giving up nominal typing is going to haunt me. But until the language has a way to have user-defined types, I think structural typing is the way to go.

src/wasm-lib/kcl/src/std/extrude.rs

src/wasm-lib/kcl/src/std/sketch.rs

Irev-Dev

Looks good to me.

(aside from the fact that some of the JS unit tests will need adjusting)

franknoirot · 2024-08-19T12:19:16Z

src/lang/wasm.ts

+  const actualType = obj?.value?.type ?? obj?.type
+  if (actualType) {
+    return new Error(
+      `Expected ${varName} to be a sketchGroup, but it ${actualType} instead.`


Suggested change

`Expected ${varName} to be a sketchGroup, but it ${actualType} instead.`

`Expected ${varName} to be a sketchGroup, but it was ${actualType} instead.`

franknoirot · 2024-08-19T12:20:31Z

src/machines/modelingMachine.ts

+          kclManager.programMemory.get(sketchVar),
+          sketchVar
+        )
+        if (trap(sketchGroup)) return


I think trap will throw up a toast, just a heads up in case that isn't the intended behavior, since I saw you use err elsewhere.

The problem is that switching to err would silently swallow the error. Do we have another function that prints to the console but doesn't show a toast?

Seems like we can use trap with suppress = true for that.

We can store Rust types like `SketchGroup` as their own variant of `KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and try to read from the other, which fails. This causes bugs, like #3338. Instead, we should use either ::SketchGroup or ::UserVal, and stop using the other. If we stopped using ::UserVal, we'd need a new variant for every Rust type we wanted to build, including user-defined types. So I don't think that's practical. Instead, we should store every KCL value by de/serializing it into UserVal. This is a first step along that path, removing just the SketchGroup variants. If it goes well, we can remove the other specialized variants too. My only concern is there might be performance implications from how frequently we convert between serde_json::Value and Rust types via Serde. But I'm not too worried -- there's no parsing JSON strings, just traversing serde_json::Value trees. This isn't great for performance but I think it'll probably be miniscule in comparison to doing all the API calls. I'll benchmark it and see.

@vipyne

* Move error-handling into the getter function, ty @vipyne * Update src/lang/wasm.ts Co-authored-by: Jonathan Tran <[email protected]> --------- Co-authored-by: Jonathan Tran <[email protected]>

adamchalmers requested review from jtran and jessfraz August 15, 2024 03:20

jessfraz approved these changes Aug 15, 2024

View reviewed changes

adamchalmers force-pushed the achalmers/no-more-kcl-sketchgroup-variants branch from a653b86 to fb28993 Compare August 15, 2024 03:42

vercel bot deployed to Preview August 15, 2024 04:03 View deployment

jtran approved these changes Aug 15, 2024

View reviewed changes

src/wasm-lib/kcl/src/std/extrude.rs Outdated Show resolved Hide resolved

src/wasm-lib/kcl/src/std/sketch.rs Outdated Show resolved Hide resolved

adamchalmers force-pushed the achalmers/no-more-kcl-sketchgroup-variants branch from 68c5940 to 386ae08 Compare August 15, 2024 13:03

vercel bot deployed to Preview August 15, 2024 13:24 View deployment

vercel bot deployed to Preview August 15, 2024 16:49 View deployment

adamchalmers force-pushed the achalmers/no-more-kcl-sketchgroup-variants branch 3 times, most recently from 0d030ae to 4a43a2e Compare August 15, 2024 19:42

vercel bot deployed to Preview August 15, 2024 19:53 View deployment

Irev-Dev approved these changes Aug 15, 2024

View reviewed changes

vercel bot deployed to Preview August 15, 2024 21:48 View deployment

vercel bot deployed to Preview August 15, 2024 23:00 View deployment

adamchalmers force-pushed the achalmers/no-more-kcl-sketchgroup-variants branch from 016df94 to c99847a Compare August 16, 2024 15:56

vercel bot deployed to Preview August 16, 2024 16:07 View deployment

adamchalmers force-pushed the achalmers/no-more-kcl-sketchgroup-variants branch 2 times, most recently from c5a1643 to a580d3a Compare August 16, 2024 17:38

vercel bot deployed to Preview August 16, 2024 17:50 View deployment

vercel bot deployed to Preview August 16, 2024 18:01 View deployment

franknoirot approved these changes Aug 19, 2024

View reviewed changes

adamchalmers added 4 commits August 21, 2024 09:56

Bugfix

33cac26

Remove dbg

722c6e1

Start resolving the Typescript errors

5a41192

adamchalmers and others added 3 commits August 21, 2024 09:59

Move error-handling into the getter function (#3468)

29fa4b3

* Move error-handling into the getter function, ty @vipyne * Update src/lang/wasm.ts Co-authored-by: Jonathan Tran <[email protected]> --------- Co-authored-by: Jonathan Tran <[email protected]>

Fix JS tests

be676bb

Fix typo

dada9f8

adamchalmers force-pushed the achalmers/no-more-kcl-sketchgroup-variants branch from f8b9ebb to dada9f8 Compare August 21, 2024 14:59

Fix formatting

ed16e62

adamchalmers force-pushed the achalmers/no-more-kcl-sketchgroup-variants branch from 5da5afb to ed16e62 Compare August 21, 2024 15:17

Fix lints

50af13b

vercel bot deployed to Preview August 21, 2024 15:57 View deployment

adamchalmers force-pushed the achalmers/no-more-kcl-sketchgroup-variants branch from 2ff3c50 to 50af13b Compare August 21, 2024 16:06

adamchalmers merged commit d656a38 into main Aug 21, 2024
42 of 53 checks passed

adamchalmers deleted the achalmers/no-more-kcl-sketchgroup-variants branch August 21, 2024 16:06

github-actions bot mentioned this pull request Aug 26, 2024

Increase PR limit KittyCAD/ts-actions#326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove KclValue::SketchGroup variant #3446

Remove KclValue::SketchGroup variant #3446

adamchalmers commented Aug 15, 2024 •

edited

Loading

qa-wolf bot commented Aug 15, 2024

vercel bot commented Aug 15, 2024 •

edited

Loading

jessfraz left a comment

codecov bot commented Aug 15, 2024 •

edited

Loading

jtran left a comment

Irev-Dev left a comment •

edited

Loading

franknoirot Aug 19, 2024

franknoirot Aug 19, 2024

jtran Aug 19, 2024

adamchalmers Aug 19, 2024

	`Expected ${varName} to be a sketchGroup, but it ${actualType} instead.`
	`Expected ${varName} to be a sketchGroup, but it was ${actualType} instead.`

Remove KclValue::SketchGroup variant #3446

Remove KclValue::SketchGroup variant #3446

Conversation

adamchalmers commented Aug 15, 2024 • edited Loading

qa-wolf bot commented Aug 15, 2024

vercel bot commented Aug 15, 2024 • edited Loading

jessfraz left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 15, 2024 • edited Loading

Codecov Report

jtran left a comment

Choose a reason for hiding this comment

Irev-Dev left a comment • edited Loading

Choose a reason for hiding this comment

franknoirot Aug 19, 2024

Choose a reason for hiding this comment

franknoirot Aug 19, 2024

Choose a reason for hiding this comment

jtran Aug 19, 2024

Choose a reason for hiding this comment

adamchalmers Aug 19, 2024

Choose a reason for hiding this comment

adamchalmers commented Aug 15, 2024 •

edited

Loading

vercel bot commented Aug 15, 2024 •

edited

Loading

codecov bot commented Aug 15, 2024 •

edited

Loading

Irev-Dev left a comment •

edited

Loading