Parse KCL measurement units consistently - #1844
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1844 +/- ##
==========================================
+ Coverage 66.11% 66.24% +0.13%
==========================================
Files 43 43
Lines 8434 8467 +33
==========================================
+ Hits 5576 5609 +33
Misses 2858 2858
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adamchalmers
left a comment
There was a problem hiding this comment.
Pls do not merge, hold for proper review
|
Please retarget this branch to deserialize command-line args for UoM via the kittycad-modeling-cmds crate (released in version 0.2.229) rather than implementing conversions between them. |
|
kcmc v0.2.229 was released with the clap derive now so you can remove the crates.io patch. Why does only volume need special treatment here, should this apply to all units? Genuine question. |
KCL measurement arguments use the native
kittycad-modeling-cmdsunit enums for volume, mass, density, surface area, center of mass, bounding boxes, and combined analysis. Enabling the crate'sclapfeature makes accepted units and invalid-value diagnostics consistent. No API-to-modeling volume conversion helper remains.Preserve existing abbreviations, analysis defaults, and density aliases. Regression coverage exercises all 14 KCL measurement-unit flags, checks the listed choices for invalid input, compares file/KCL volume spellings, and checks all six density spellings across the four density-unit flags.
The workspace-wide Cargo patch remains pinned to
52ecdb59580435964776ad458e2036b73a4ef7d7so the CLI and KCL dependencies share the same modeling-command types. Published0.2.229includes #1369 but not the compatibility fixes in modeling-api#1370: it omits volume'sValueEnumand derives long unit names that reject the CLI's abbreviations and defaults. Removing the patch and selecting0.2.229reproduces three failures in the original seven-test suite. #1370 is now merged, but its fixes are not yet in a published crate version. Update the dependency and remove the patch after that release.Validation against the pinned dependency:
cargo test cmd_kcl::tests:: --locked --offline: 7 passed, covering all 14 unit flags and density aliases.cargo clippy --all-features --all-targets --locked --offline -- -D warnings: passed.cargo fmt --all -- --check: passed.git diff --check: passed.These checks exercise local command parsing; they do not run live Engine or API requests.
Fixes #1744