-
Notifications
You must be signed in to change notification settings - Fork 74
Use typed KclValue instead of any in sketchFromKclValue #7144
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
Use typed KclValue instead of any in sketchFromKclValue #7144
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/lang/wasm.ts
Outdated
if (valueType === 'Sketch') return obj?.value as Sketch | ||
if (valueType === 'Solid') return (obj?.value as any)?.sketch | ||
if (obj?.type === 'Sketch') return obj.value | ||
if (obj?.type === 'Solid') return obj.value.sketch |
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.
When is it possible that obj.value.type
is not the same as obj.type
? According to the code it seems obj.value.type
can be 'Sketch'
while obj.type
isn't.
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.
The internal structure of KclValue has changed a lot since this code was written. I think that if this can type check without any
, and there aren't crazy uses of any
where this is called, then it should be fine to delete impossible cases.
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.
Is the source of truth for the type of KclValue this rust code?
https://github.com/KittyCAD/modeling-app/blob/4d404bf137975e04f327df2c411dbb36dd856d08/rust/kcl-lib/src/execution/kcl_value.rs
It looks like the TS type is generated automatically so I assume those types are correct, but is it still possible that some data is using an older format? If not, I guess we could delete these lines:
if (valueType === 'Sketch') return obj?.value as Sketch
if (valueType === 'Solid') return (obj?.value as any)?.sketch
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.
Yes, that's the source of truth. We use something called ts-rs that generates the TS types from the Rust source.
No, we don't serialize and store KclValue anywhere. It's all within a single run of the app.
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.
I actually this can be merged, I remove all any
s, I haven't any cases where obj.type
would differ from obj.value.type
, the tests are passing..
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.
Nice! Thank you!
src/lang/wasm.ts
Outdated
if (obj?.type === 'Sketch') return obj.value | ||
if (obj?.type === 'Solid') return obj.value.sketch | ||
if (!varName) { | ||
varName = 'a KCL value' | ||
} | ||
const actualType = obj?.value?.type ?? obj?.type | ||
|
||
const actualType = obj?.type ?? 'unknown' | ||
if (actualType) { |
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 if-else can probably go away too since you're using ?? 'unknown'
.
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.
Oh yes, thank you!
#7143
Not wanting to merge yet, just starting a discussion about how to fix properly.