Skip to content

Support partial unit systems#1690

Merged
angularsen merged 1 commit into
masterfrom
agl/agl-partial-unit-systems
Jul 22, 2026
Merged

Support partial unit systems#1690
angularsen merged 1 commit into
masterfrom
agl/agl-partial-unit-systems

Conversation

@angularsen

Copy link
Copy Markdown
Owner

Extracted from #1544 because representing a unit system for only the dimensions an application uses is useful independently of QuantityValue. Moving it out gives the API change focused review and reduces the original PR's scope.

Changes:

  • Allow UnitSystem to be constructed from any BaseUnits value defining at least one dimension.
  • Continue rejecting null and fully undefined base-unit sets.
  • Clarify the constructor documentation and exception message.

Tests:

  • Change the seven previously rejected one-dimension-missing cases to assert successful construction.
  • Add a single-dimension UnitSystem case.
  • Add a dedicated assertion that BaseUnits.Undefined remains invalid.

Extracted from #1544 (#1544) because representing a unit system for only the dimensions an application uses is useful independently of QuantityValue. Moving it out gives the API change focused review and reduces the original PR's scope.

Changes:
- Allow UnitSystem to be constructed from any BaseUnits value defining at least one dimension.
- Continue rejecting null and fully undefined base-unit sets.
- Clarify the constructor documentation and exception message.

Tests:
- Change the seven previously rejected one-dimension-missing cases to assert successful construction.
- Add a single-dimension UnitSystem case.
- Add a dedicated assertion that BaseUnits.Undefined remains invalid.
- Verify all UnitSystemTests on net10.0 and net48 (16 tests per target).
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review of #1690 — Support partial unit systems

Small, focused, well-tested change. Summary of what I checked:

Breaking changes

None. The change only relaxes validation in UnitSystem(BaseUnits): previously it required baseUnits.IsFullyDefined, now it only rejects BaseUnits.Undefined (all 7 dimensions null). Any code that previously constructed a UnitSystem successfully still works identically; only previously-throwing calls (partial base units) now succeed. The only way this could bite someone is if they had a try/catch (ArgumentException) around a partial-BaseUnits construction expecting failure — unlikely and not a supported pattern.

Style & conventions

Matches existing conventions in UnitSystem.cs and UnitSystemTests.cs. Doc comment update (One or more base units that define the unit system.) and the new exception message ("A unit system must define at least one base unit.") are both accurate and read well.

New quantities/units

N/A — no new quantities/units added.

Generated code

N/A — no changes to Common/UnitDefinitions/*.json or CodeGen/, and no generated files are touched.

Code quality / correctness

Verified the change is consistent with how UnitSystem.BaseUnits is actually consumed downstream, in UnitInfo.GetUnitsWithBase/BaseUnits.IsSubsetOf (UnitInfo.cs:61, BaseUnits.cs:86-101). That matching logic already treats a unit's own undefined dimensions as "don't care," and requires the unit's defined dimensions to match the target BaseUnits — so allowing a partially-defined UnitSystem composes correctly: it simply narrows matches to units whose relevant dimensions are satisfied, with no risk of over-matching. Nice that this holds up against real consumer code, not just the constructor in isolation.

Potential bugs

None found. The baseUnits == BaseUnits.Undefined check reuses BaseUnits's existing null-safe == operator correctly (the null-guard for baseUnits itself happens first in the constructor).

Performance

Negligible — same single equality check as before.

Test coverage

Good coverage of the new boundary:

  • All seven "exactly one dimension missing" cases now assert successful construction (previously asserted failure) — correctly renamed to ConstructorSupportsPartialDimensions.
  • Added a single-dimension-only case (LengthUnit.Meter with everything else null).
  • Added a dedicated ConstructorThrowsArgumentExceptionWithUndefinedUnits test asserting BaseUnits.Undefined is still rejected.

One optional suggestion: it might be worth a test (here or in a follow-up) exercising QuantityInfoExtensions.GetDefaultUnit/GetUnitInfosFor with a genuinely partial UnitSystem (e.g. only Length defined) to lock in the downstream matching behavior described above, since that's the actual motivating use case per the PR description.

Security

No concerns.

Overall: a clean, minimal, low-risk change that does what the description says. Good call extracting this out of #1544 for focused review.

@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96%. Comparing base (7e8da49) to head (54808ca).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #1690    +/-   ##
=======================================
  Coverage      96%     96%            
=======================================
  Files         456     454     -2     
  Lines       30107   29497   -610     
=======================================
- Hits        29043   28459   -584     
+ Misses       1064    1038    -26     
Flag Coverage Δ
net48 ?

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.

@angularsen
angularsen merged commit 246b1cb into master Jul 22, 2026
5 of 6 checks passed
@angularsen
angularsen deleted the agl/agl-partial-unit-systems branch July 22, 2026 23:33
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.

1 participant