Skip to content

Parse KCL measurement units consistently - #1844

Open
JordanNoone wants to merge 4 commits into
mainfrom
codex/volume-units-1744
Open

Parse KCL measurement units consistently#1844
JordanNoone wants to merge 4 commits into
mainfrom
codex/volume-units-1744

Conversation

@JordanNoone

@JordanNoone JordanNoone commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

KCL measurement arguments use the native kittycad-modeling-cmds unit enums for volume, mass, density, surface area, center of mass, bounding boxes, and combined analysis. Enabling the crate's clap feature 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 52ecdb59580435964776ad458e2036b73a4ef7d7 so the CLI and KCL dependencies share the same modeling-command types. Published 0.2.229 includes #1369 but not the compatibility fixes in modeling-api#1370: it omits volume's ValueEnum and derives long unit names that reject the CLI's abbreviations and defaults. Removing the patch and selecting 0.2.229 reproduces 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

@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.24%. Comparing base (0f4b8a0) to head (e2e7d6a).

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              
Flag Coverage Δ
unittests 66.24% <100.00%> (+0.13%) ⬆️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JordanNoone
JordanNoone marked this pull request as ready for review September 6, 2026 04:10

@adamchalmers adamchalmers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pls do not merge, hold for proper review

@adamchalmers

adamchalmers commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

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.

@adamchalmers

Copy link
Copy Markdown
Contributor

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.

@JordanNoone JordanNoone changed the title Align file and KCL volume unit parsing Parse KCL measurement units consistently Sep 7, 2026
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.

Volume unit enums disagree: zoo kcl volume accepts mm3, zoo file volume rejects it

2 participants