Skip to content

Add quantity selection factories#1691

Merged
angularsen merged 2 commits into
masterfrom
agl/quantity-selection-factories
Jul 23, 2026
Merged

Add quantity selection factories#1691
angularsen merged 2 commits into
masterfrom
agl/quantity-selection-factories

Conversation

@angularsen

Copy link
Copy Markdown
Owner

Extracted from #1544 because composing an isolated quantity catalog is useful for custom quantities without depending on QuantityValue. Moving this API out gives the catalog design focused review and reduces the feature PR's size and initialization surface.

Changes:

  • Add QuantitiesSelector for appending external quantity definitions to a base catalog.
  • Add configured factory overloads for QuantityInfoLookup, UnitAbbreviationsCache, and UnitParser.
  • Allow parser factories to configure abbreviations before parser construction.
  • Add UnitsNetSetup.Create and a selection builder for isolated, consistently wired setups.
  • Keep UnitsNetSetup.Default behavior unchanged.

Tests:

  • Add QuantitiesSelector coverage for base, chained additions, ordering, and null input.
  • Add cache factory coverage for default and custom base catalogs.
  • Add parser factory coverage for external quantities and custom abbreviations.
  • Add isolated setup coverage for component wiring, catalog selection, additions, and duplicate base selection.

Extracted from #1544 (#1544) because composing an isolated quantity catalog is useful for custom quantities without depending on QuantityValue. Moving this API out gives the catalog design focused review and reduces the feature PR's size and initialization surface.

Changes:
- Add QuantitiesSelector for appending external quantity definitions to a base catalog.
- Add configured factory overloads for QuantityInfoLookup, UnitAbbreviationsCache, and UnitParser.
- Allow parser factories to configure abbreviations before parser construction.
- Add UnitsNetSetup.Create and a selection builder for isolated, consistently wired setups.
- Keep UnitsNetSetup.Default behavior unchanged.

Tests:
- Add QuantitiesSelector coverage for base, chained additions, ordering, and null input.
- Add cache factory coverage for default and custom base catalogs.
- Add parser factory coverage for external quantities and custom abbreviations.
- Add isolated setup coverage for component wiring, catalog selection, additions, and duplicate base selection.
- Verify all affected tests on net10.0 and net48 (118 tests per target).
@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review

Scope: purely additive (321 additions / 0 deletions), no changes to generated code or Common/UnitDefinitions/*.json, no new quantities/units. This is a C# API surface addition for composing custom UnitsNetSetup/UnitParser/UnitAbbreviationsCache instances via a new QuantitiesSelector.

Breaking changes

None found. All changes are new members; UnitsNetSetup.Default construction path (static ctor) is untouched, matching the PR description.

Design concern: UnitConverter is not actually scoped by the selection

UnitsNetSetup.DefaultConfigurationBuilder.Build() (UnitsNet/CustomCode/UnitsNetSetup.cs:89-93) always does:

IEnumerable<QuantityInfo> quantities = _quantitiesSelector?.GetQuantityInfos() ?? Quantity.DefaultProvider.Quantities;
return new UnitsNetSetup(quantities, UnitConverter.CreateDefault());

UnitConverter.CreateDefault() (UnitsNet/UnitConverter.cs:62-68) unconditionally registers conversion functions for all ~130 built-in quantities via Quantity.DefaultProvider.RegisterUnitConversions, completely independent of whatever was selected/excluded. Two consequences:

  1. Reducing the catalog doesn't reduce the converter. UnitsNetSetup.Create(b => b.WithQuantities([Mass.Info])) yields a Quantities/UnitAbbreviations/UnitParser that reject Length (per the test Create_WithSelectedQuantities_WiresIsolatedComponentsToSameCatalog, which asserts Quantities.GetQuantityByUnitType(typeof(LengthUnit)) throws), but setup.UnitConverter still has full Length conversion functions registered and would happily convert them. The four components of the "isolated" setup are inconsistent with each other.
  2. Adding external quantities doesn't wire up their conversions. For the PR's stated main use case — WithAdditionalQuantities([HowMuch.Info])Quantities, UnitAbbreviations, and UnitParser all recognize the custom quantity, but UnitConverter gets no conversion functions for it (there's no generic mechanism to derive them from QuantityInfo; built-ins are wired via generated per-type RegisterDefaultConversions calls). A caller would need to manually populate setup.UnitConverter afterward for UnitConverter.Convert/ConvertByAbbreviation to work with the new quantity, even though the setup otherwise looks fully wired.

Worth at least a doc-comment caveat on UnitsNetSetup.Create/DefaultConfigurationBuilder, if not deferred to the follow-up PR (#1544) intentionally.

Naming

UnitsNetSetup.DefaultConfigurationBuilder reads oddly once you use WithQuantities(...) to replace the default catalog with a custom/reduced one — "Default" in the name suggests it's only for tweaking the default set. Something like SetupBuilder/UnitsNetSetupBuilder would better match its actual role (it's also used for full replacement, not just built-in + additions).

Test coverage gaps

The codebase's existing convention (e.g. UnitParserTests, UnitAbbreviationsCacheTests) is to test ArgumentNullException guard clauses for public API entry points. The new public factory methods lack these:

  • UnitsNetSetup.Create(null)
  • UnitParser.CreateDefault(null) / UnitParser.Create(null, ...) / UnitParser.Create(..., configureAbbreviations: null)
  • UnitAbbreviationsCache.CreateDefault(null) / UnitAbbreviationsCache.Create(null, ...)

Only QuantitiesSelectorTests.WithAdditionalQuantities_WithNull_ThrowsArgumentNullException covers this pattern currently.

Also no test for duplicate quantities across WithQuantities/WithAdditionalQuantities (e.g. accidentally adding a quantity already present in the base catalog). QuantityInfoLookup's lazy ToDictionary calls (GroupQuantitiesByName/GroupQuantitiesByType/GroupQuantitiesByUnitType) will throw a generic "duplicate key" ArgumentException on first use of the lookup rather than immediately at selection time — likely pre-existing behavior of QuantityInfoLookup, but this PR is what makes accidental duplication a realistic user-facing scenario, so a guard or at least a test documenting the current (deferred, cryptic) failure mode would help.

Style

Everything else matches repo conventions well: XML doc comments, manual is null guard clauses (consistent with multi-targeting, no ArgumentNullException.ThrowIfNull), flat UnitsNet namespace regardless of folder nesting, <Method>_<condition>_<result> test naming, explicit using System.Linq;.

Minor

  • QuantitiesSelector is public but its only "read" method, GetQuantityInfos(), is internal — so external consumers can chain .WithAdditionalQuantities(...) but can only get a result back through the blessed factory methods. Seems intentional (keeps the type usable only as a fluent config object), just flagging so it's confirmed deliberate rather than an oversight.
  • UnitParser.Create(defaultQuantities, configureQuantities, configureAbbreviations) null-checks configureAbbreviations directly but relies on UnitAbbreviationsCache.CreateQuantityInfoLookup.Create to null-check defaultQuantities/configureQuantities. Fine functionally, just means the null checks for the same method aren't co-located, which reads slightly inconsistently, e.g. compared to other overloads that check inline.

Overall this is a clean, well-tested, low-risk additive change — the main thing worth resolving before merge (or explicitly deferring) is the UnitConverter scoping gap, since "isolated, consistently wired setups" is the PR's stated goal and the converter component currently isn't consistent with the other three.

@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 (f15fac7).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #1691    +/-   ##
=======================================
  Coverage      96%     96%            
=======================================
  Files         456     455     -1     
  Lines       30107   29540   -567     
=======================================
- Hits        29043   28502   -541     
+ 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.

Cover the setup and parser factory overloads missed by Codecov, and verify null argument contracts across the new public factory APIs.
@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review: Add quantity selection factories (#1691)

Scope: purely additive custom-code API (no changes to Common/UnitDefinitions/*.json or generated code, no new quantities/units), so most of the standard checklist doesn't apply here. Focused on the new builder API in UnitsNet/CustomCode/.

Breaking changes

None. All changes are new public members (new QuantitiesSelector class, new static overloads on UnitAbbreviationsCache/UnitParser/QuantityInfoLookup, new UnitsNetSetup.Create/DefaultConfigurationBuilder). UnitsNetSetup.Default behavior is untouched, as the PR description states.

Potential bug: UnitsNetSetup.Create doesn't actually isolate the UnitConverter

DefaultConfigurationBuilder.Build() always calls UnitConverter.CreateDefault() regardless of the quantities selected:

internal UnitsNetSetup Build()
{
    IEnumerable<QuantityInfo> quantities = _quantitiesSelector?.GetQuantityInfos() ?? Quantity.DefaultProvider.Quantities;
    return new UnitsNetSetup(quantities, UnitConverter.CreateDefault());
}

UnitConverter.CreateDefault() registers conversion functions for all built-in quantities (UnitConverter.cs:62-68RegisterDefaultConversionsQuantity.DefaultProvider.RegisterUnitConversions), independent of whatever subset was passed to WithQuantities/WithAdditionalQuantities. Two consequences:

  • An "isolated" setup built with e.g. UnitsNetSetup.Create(b => b.WithQuantities([Mass.Info])) still carries conversion functions for the other ~130 quantities in its UnitConverter — not actually isolated, just the parser/abbreviations/lookup are scoped.
  • More importantly, an external/custom quantity added via WithAdditionalQuantities (the PR's stated motivating use case) gets no conversion functions registered at all, since there's no hook to feed custom conversions into the UnitConverter. Quantities/UnitParser/UnitAbbreviations will know about the custom quantity, but setup.UnitConverter.Convert(...) will silently fail/throw for it. Worth either scoping UnitConverter to the selected quantities, exposing a way to configure it (mirroring configureAbbreviations on UnitParser.Create), or documenting the limitation explicitly.
  • Side effect: every call to UnitsNetSetup.Create(...) pays the cost of compiling delegates for the full default conversion graph, even when the caller only wants a couple of quantities — works against the "lightweight isolated catalog" goal from the PR description.

None of the new tests exercise setup.UnitConverter for isolated/custom-quantity setups, which is likely why this wasn't caught.

Bug: builder is order-dependent in a surprising way

DefaultConfigurationBuilder.WithAdditionalQuantities lazily initializes _quantitiesSelector if it's null:

public DefaultConfigurationBuilder WithAdditionalQuantities(IEnumerable<QuantityInfo> quantities)
{
    _quantitiesSelector ??= new QuantitiesSelector(() => Quantity.DefaultProvider.Quantities);
    _quantitiesSelector.WithAdditionalQuantities(quantities);
    return this;
}

If a caller calls WithAdditionalQuantities(...) before WithQuantities(...), the subsequent WithQuantities(...) call throws InvalidOperationException("The base quantity selection is already configured.") — even though the caller never explicitly called WithQuantities. E.g.:

UnitsNetSetup.Create(b => b
    .WithAdditionalQuantities([HowMuch.Info])
    .WithQuantities([Mass.Info])); // throws InvalidOperationException, surprising to the caller

The tests only cover WithQuantitiesWithAdditionalQuantities order; the reverse isn't tested and the error message doesn't hint that WithAdditionalQuantities is what "configured" the base selection. Consider either making the two composable regardless of order, or documenting/guarding this explicitly (e.g. a clearer exception message).

Style/naming

  • UnitsNetSetup.DefaultConfigurationBuilder is a slightly confusing name — it doesn't build the Default setup and isn't related to UnitsNetSetup.Default; it's a general builder for any isolated setup. Something like SetupBuilder or ConfigurationBuilder would avoid the association.
  • The overload surface across UnitAbbreviationsCache, UnitParser, QuantityInfoLookup, and UnitsNetSetup.DefaultConfigurationBuilder is fairly large (several Create/CreateDefault/WithQuantities overloads) for what's a narrow use case. Not blocking, but worth double-checking all combinations are actually needed vs. a smaller, more orthogonal set (e.g. base catalog + configure action, always).

Code quality

  • QuantitiesSelector's internal constructor and internal GetQuantityInfos() correctly prevent external code from constructing/reading it directly except through the exposed Action<QuantitiesSelector> configuration callbacks — good encapsulation.
  • Null-checks consistently use the repo's if (x is null) throw new ArgumentNullException(nameof(x)); style, and error param names are verified by tests (FactoryMethods_WithNullArguments_ThrowArgumentNullException) — nice attention to detail.
  • QuantityInfoLookup.Create is internal, appropriately not part of the public surface directly.

Test coverage

Good coverage for the new selector/builder logic itself (ordering, chaining, null args, duplicate WithQuantities calls). The gap is functional/integration coverage of the resulting UnitConverter for isolated setups — see the bug above.

Overall this is a clean, well-tested, additive change to the custom-code layer, but the UnitConverter isolation gap and the builder ordering issue seem worth addressing (or at least documenting) before merge.

@angularsen
angularsen merged commit 3318f70 into master Jul 23, 2026
5 of 6 checks passed
@angularsen
angularsen deleted the agl/quantity-selection-factories branch July 23, 2026 00:06
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