Fix ambiguous prefixed cubic units - #1700
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1700 +/- ##
======================================
Coverage 96% 96%
======================================
Files 457 457
Lines 29310 29301 -9
======================================
- Hits 28247 28239 -8
+ Misses 1063 1062 -1
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:
|
1bdb3a1 to
537b546
Compare
Review: Fix ambiguous prefixed cubic unitsReviewed via Breaking changesThis is correctly a v6-only change (enum renames, abbreviation reassignment, and one silent value fix), and the PR does the right things around it:
New unitsThe three new count-style cubic-foot units ( One nit: Generated codeSpot-checked
Note on scope: This PR fixes the JSON definitions for Test coverageGood regression tests added:
Style/docs
No security concerns and no performance concerns beyond the existing compiled-delegate conversion approach, which is unaffected here. |
## Motivation Follow-up to #1700. We should fail codegen when `Prefixes` is used on units that look like powered units themselves, such as `SquareMeter`, `CubicMeter`, or abbreviations starting with `m²`, `m³`, `ft³`, and similar symbols. That avoids reintroducing bugs like `KilocubicMeter` formatting as `km³`, where the generated prefix looks like it applies before the length unit is cubed. This is intentionally narrower than checking base dimensions. Units like `Megawatt`, `Megajoule`, and `Megaohm` still need to be allowed even though their SI base dimensions contain exponents internally. ## Changes - Add a CodeGen guard for `Prefixes` on units that look like powered units. - Keep existing valid derived-unit prefixes working. - Document that this rule is now enforced by CodeGen. ## Validation - `dotnet build CodeGen\CodeGen.csproj` - `generate-code.bat` - Temporarily added `Prefixes: [ "Kilo" ]` to `Area.SquareMeter` and verified CodeGen fails with the new error, then reverted the temporary edit.
Motivation
Fixes #1617 and #1645.
Some cubic units used
Prefixeswhere the prefix only applied to the generated unit name, not to the cubed length unit. That made abbreviations such askm³,hm³, andmm³/kgambiguous or wrong:km³/hm³should mean cubic kilometers/hectometers, not 1000 or 100 cubic meters.mm³/kgshould mean cubic millimeters per kilogram, not millesimal cubic meters per kilogram.This follows the SI prefix model where the prefixed unit symbol can itself be raised to a power: https://www.bipm.org/en/measurement-units/si-prefixes
The original intent for the generated cubic foot/meter count units seems to have been real-world count-style volume units such as
kcfandKcm, rather than true cubic-prefixed SI units. This PR keeps those concepts, but gives them explicit names and abbreviations so they do not conflict with true prefixed cubic length units.For cubic feet, this uses the common gas-volume forms
Ccf,Mcf, andMMcf. EIA listsCcfas 100 cubic feet andMcfas 1000 cubic feet: https://www.eia.gov/tools/faqs/faq.php?id=45&t=8For thousand cubic meters, this uses
10³·m³as the primary abbreviation and keepskcm/Kcmas aliases. ENERGY STAR Portfolio Manager listsKcmas thousand cubic meters.Changes
SpecificVolumeUnit.MillicubicMeterPerKilogramwithSpecificVolumeUnit.CubicMillimeterPerKilogram.VolumeUnit.HectocubicMeter; useVolumeUnit.CubicMeterfor 100 m³ values.VolumeUnit.KilocubicMeterwithVolumeUnit.ThousandCubicMeter.VolumeUnit.HectocubicFoot,KilocubicFoot, andMegacubicFootwithHundredCubicFoot,ThousandCubicFoot, andMillionCubicFoot.Migration notes
hm³andkm³now mean true cubic hectometers and cubic kilometers.mm³/kgnow means true cubic millimeters per kilogram.Volume.FromCubicMeters(100)or convert withVolumeUnit.CubicMeter.Validation
generate-code.batdotnet test UnitsNet.Tests --filter "FullyQualifiedName~UnitsNet.Tests.VolumeTests|FullyQualifiedName~UnitsNet.Tests.CustomCode.SpecificVolumeTests"