Skip to content

Commit d012ee2

Browse files
angularsenangularsen
committed
provide two alternatives for avoiding big breaking changes
1 parent 7720782 commit d012ee2

1 file changed

Lines changed: 152 additions & 18 deletions

File tree

‎review_pr1544.md‎

Lines changed: 152 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,156 @@
11
# PR #1544 Review: QuantityValue as Fractional Number
22

3-
**Review date**: 2026-03-09 | **Branch**: `fractional-quantity-value`
3+
**Review date**: 2026-03-09, updated 2026-03-26 | **Branch**: `fractional-quantity-value`
44

55
---
66

7-
## Changes Required Before Merge
7+
## P0 — Reducing the breaking change impact of `QuantityValue`
88

9-
### P0 — Must do
9+
The PR replaces `double` with `QuantityValue` (BigInteger-backed rational) across the entire public API. Without mitigation, ~60-70% of consumer code breaks on upgrade. We propose two alternative solutions.
1010

11-
1. **Add implicit conversion `QuantityValue` → `double`**
12-
Most consumers just want a `double`. Without this, ~60-70% of consumer code breaks on upgrade. With implicit conversion, that drops to ~20-30%. Those who want exact precision can still use `QuantityValue` directly.
11+
### Summary
1312

14-
2. **Revert `EmitDefaultValue = false` on DataMember**
13+
| | Option A: Add implicit cast | Option B: Keep `double` public |
14+
|---|---|---|
15+
| **Approach** | Add `implicit operator double(QuantityValue)` | Revert public API to `double`, use `QuantityValue` internally for conversions |
16+
| **Rework effort** | Minimal — one operator addition | Significant — revert generated types, adjust CodeGen templates |
17+
| **PR value preserved** | 100% | ~90% |
18+
| **Consumer breaking changes** | Eliminated for ~60-70% of code | Eliminated for ~60-70% of code |
19+
| **Precision model** | Consumers store and pass `QuantityValue` (full precision until they convert to `double`) | Consumers always work with `double`; precision only during conversion pipeline |
20+
| **Risk** | Bidirectional implicit conversion is unusual in C#; may surface edge cases we haven't identified | More conservative; well-understood behavior |
21+
| **`var x = len.Meters`** | `x` is `QuantityValue` (but implicitly converts to `double` wherever needed) | `x` is `double` |
22+
| **Future flexibility** | Consumers who want exact precision can use `QuantityValue` directly today | Would require a later API change to expose `QuantityValue` to consumers |
23+
24+
**Our recommendation: Option A.** It's simpler, preserves the full PR, and our analysis found no concrete compilation failures. Option B is a solid fallback if unforeseen issues arise.
25+
26+
---
27+
28+
## Option A: Add implicit conversion `QuantityValue → double`
29+
30+
### What changes
31+
32+
One line added to `QuantityValue.ConvertToNumber.cs`:
33+
34+
```csharp
35+
public static implicit operator double(QuantityValue value) => value.ToDouble();
36+
```
37+
38+
The existing `explicit` keyword changes to `implicit`. Everything else in the PR stays as-is.
39+
40+
### Why it works
41+
42+
The PR is clean — **no generated operator takes `double`**. Every operator uses `QuantityValue`:
43+
44+
```csharp
45+
// Generated operators (e.g. Length.g.cs):
46+
Length operator *(QuantityValue left, Length right)
47+
Length operator *(Length left, QuantityValue right)
48+
Length operator /(Length left, QuantityValue right)
49+
QuantityValue operator /(Length left, Length right)
50+
```
51+
52+
`QuantityValue` arithmetic operators are all `(QuantityValue, QuantityValue)` — no mixed-type overloads.
53+
54+
With bidirectional implicit conversions (`double ↔ QuantityValue`), C# overload resolution prefers user-defined operators over built-in ones when both require one conversion. So `QV + double` resolves to user-defined `QV + QV` (promoting `double`), not built-in `double + double` (demoting `QV`).
55+
56+
### Scenario analysis
57+
58+
| Scenario | Works? | Explanation |
59+
|----------|--------|-------------|
60+
| `double meters = length.Meters` | Yes | `QuantityValue` implicitly converts to `double` |
61+
| `double ratio = len1 / len2` | Yes | Operator returns `QV`, implicit converts to `double` |
62+
| `Math.Round(length.Meters, 2)` | Yes | Resolves to `Math.Round(double, int)` via implicit |
63+
| `SomeDoubleApi(length.Meters)` | Yes | Implicit conversion handles the call |
64+
| `Length * double` | Yes | `double` promotes to `QV`, uses `Length * QV` operator |
65+
| `QV + double` | Yes | C# prefers user-defined `QV+QV` over built-in `double+double` |
66+
| `double x = double + QV` | Yes | Operator returns `QV`, implicit converts for assignment |
67+
| `var x = length.Meters` | `x` is `QV` | Not a problem — `QV` implicitly converts wherever `double` is expected |
68+
| `QV == double` | Yes | User-defined `QV==QV` is preferred by C# overload resolution |
69+
70+
### Pros
71+
72+
- **Minimal rework** — one operator change, rest of PR untouched
73+
- **Preserves 100% of PR value** — all architecture, all new features
74+
- **Future-proof** — consumers who want exact precision can use `QuantityValue` directly; those who don't never notice the difference
75+
- **Gradual adoption** — consumers can migrate to `QuantityValue` at their own pace
76+
77+
### Cons
78+
79+
- **Bidirectional implicit conversion is unusual** — while our analysis found no issues, there may be edge cases in consumer code we can't predict (e.g. complex overload resolution scenarios)
80+
- **`var` infers `QuantityValue`** — not a functional problem (implicit conversion kicks in), but consumers see `QuantityValue` in IDE tooltips, debugger, etc.
81+
- **Silent precision loss** — converting `QuantityValue` to `double` is lossy, and implicit makes it invisible. This is the *desired* trade-off for most consumers, but purists may object.
82+
- **lipchev has stated this is a "no-go"** — may need to be verified empirically by building locally with the change
83+
84+
### Verification step
85+
86+
Build the solution locally after changing `explicit` to `implicit` in `QuantityValue.ConvertToNumber.cs`. If it compiles and tests pass, the analysis is confirmed and this is the path forward.
87+
88+
---
89+
90+
## Option B: Keep `double` public, use `QuantityValue` internally
91+
92+
### What changes
93+
94+
- `IQuantity.Value` reverts to `double`
95+
- Generated quantity unit properties (`.Meters`, `.Centimeters`, etc.) revert to `double`
96+
- Generated quantity `_value` field reverts to `double`
97+
- Scalar operators revert to `double` (`Length * double`, `Length / Length → double`)
98+
- `From()` factory methods accept `double`
99+
- `QuantityValue` remains as an internal type used by the conversion pipeline
100+
- `ConversionExpression`, `UnitConverter`, `UnitsNetSetup` builder — all unchanged
101+
102+
### How the conversion pipeline works
103+
104+
```
105+
double input → QuantityValue(input) × exact_rational_coefficient → .ToDouble() → double output
106+
```
107+
108+
The precision benefit is preserved: conversion factors like `1250/381` (feet-to-meters) are exact rationals. The stored value is `double`, and the conversion pipeline temporarily promotes to `QuantityValue` for exact arithmetic, then converts back.
109+
110+
### What's preserved from the PR (~90%)
111+
112+
| Component | Status |
113+
|-----------|--------|
114+
| `QuantityValue` type (BigInteger rational) | Preserved for internal use |
115+
| `ConversionExpression` model with exact rational coefficients | Fully preserved |
116+
| `UnitConverter` / `FrozenQuantityConverter` / `DynamicQuantityConverter` | Fully preserved |
117+
| `UnitInfo.ConversionFromBase` / `ConversionToBase` | Fully preserved |
118+
| `UnitsNetSetup` builder pattern | Fully preserved |
119+
| CodeGen rational coefficient generation | Fully preserved |
120+
| `UnitsNet.Serialization.SystemTextJson` package | Preserved |
121+
122+
### What's lost (~10%)
123+
124+
- Consumers cannot work with `QuantityValue` directly — no exact precision in user-facing code
125+
- Intermediate arithmetic between quantities uses `double`, not rationals (precision only in conversion factors)
126+
- Would require a later API change to expose `QuantityValue` to consumers who want it
127+
128+
### Pros
129+
130+
- **Zero risk** — `double` public API is well-understood, no edge cases
131+
- **No consumer breaking changes from the type change** — all `double`-based code works as before
132+
- **Familiar to consumers** — no new type to learn about
133+
- **Smaller memory footprint** — `double` is 8 bytes vs `QuantityValue` (16+ bytes for two BigInteger fields)
134+
135+
### Cons
136+
137+
- **Significant rework** — CodeGen templates, generated types, interface definitions all need adjustment
138+
- **Loses future flexibility** — consumers can't access `QuantityValue` precision without a later API change
139+
- **Precision only in conversion pipeline** — consumer arithmetic (`length1 + length2`) uses `double`, not rationals
140+
- **~10% of PR value lost** — the `QuantityValue`-as-public-type feature and its test coverage
141+
142+
---
143+
144+
## Other Changes Required Before Merge
145+
146+
### P1 — Must do
147+
148+
1. **Revert `EmitDefaultValue = false` on DataMember**
15149
Remove the `EmitDefaultValue = false` parameter from `[DataMember]` on `_value` and `_unit` fields in generated quantities. `Length.Zero` should always serialize with both Value and Unit present.
16150

17-
### P1 — Should do
151+
### P2 — Should do
18152

19-
3. **Document DataContract serialization as a breaking change**
153+
2. **Document DataContract serialization as a breaking change**
20154
State in v6 release notes that DataContract serialization format changed (XML and JSON). XML surrogate exists and works. DataContractJsonSerializer surrogate is blocked by a .NET runtime bug — recommend migrating to JsonNet or System.Text.Json packages.
21155

22156
---
@@ -25,33 +159,33 @@
25159

26160
### Before stable release
27161

28-
4. **Create migration guide** for breaking changes in this PR. Will need re-evaluation for full v6 migration guide later.
29-
5. **Add test coverage for `InterfaceQuantityWithUnitTypeConverter`** — currently appears unused/untested.
162+
3. **Create migration guide** for breaking changes in this PR. Will need re-evaluation for full v6 migration guide later.
163+
4. **Add test coverage for `InterfaceQuantityWithUnitTypeConverter`** — currently appears unused/untested.
30164

31165
### Low priority
32166

33-
6. **QuantityGenerator.cs** — Add example code comments in generated code sections for readability.
34-
7. **QuantityValueFormatOptions.cs** — Align serialization/deserialization enum names (`DecimalPrecision` vs `ExactNumber` should use consistent naming).
35-
8. **QuantityInfoBuilderExtensions** — `Configure` extension on `UnitDefinition[]` is awkward API; consider wrapper type or static method.
167+
5. **QuantityGenerator.cs** — Add example code comments in generated code sections for readability.
168+
6. **QuantityValueFormatOptions.cs** — Align serialization/deserialization enum names (`DecimalPrecision` vs `ExactNumber` should use consistent naming).
169+
7. **QuantityInfoBuilderExtensions** — `Configure` extension on `UnitDefinition[]` is awkward API; consider wrapper type or static method.
36170

37171
### Won't do
38172

39173
- Roslyn analyzer for migration assistance.
174+
- Commented code cleanup (already mostly done, 6 lines remain in CodeGen internal code).
40175

41176
---
42177

43-
## Reference: Breaking Changes Summary
178+
## Reference: Remaining Breaking Changes
179+
180+
These apply regardless of which option is chosen for `QuantityValue`:
44181

45182
| Change | Before (v5) | After (v6) |
46183
|--------|-------------|------------|
47-
| `IQuantity.Value` | `double` | `QuantityValue` |
48-
| Unit properties (e.g. `.Meters`) | `double` | `QuantityValue` |
49-
| `Length / Length` | `double` | `QuantityValue` |
50184
| `As()`, `ToUnit()` | Interface methods | Extension methods (some `[Obsolete]`) |
51185
| `UnitConverter()` constructor | Public, parameterless | Removed; use `UnitConverter.Create(...)` |
52186
| `SetConversionFunction` / `GetConversionFunction` | Available | Removed |
53187
| `UnitsNetSetup` constructor | Public | Private; use builder pattern |
54-
| DataContract serialization | `double` field | `QuantityValue` struct (numerator/denominator) |
188+
| DataContract serialization | `double` field | Changed format |
55189
| `AbbreviatedUnitsConverter` (JsonNet) | `IReadOnlyDictionary` constructor | `UnitParser` + `QuantityValueFormatOptions` |
56190
| Default JSON precision | ~17 significant digits | Up to 29 significant digits |
57191
| `MissingMemberHandling.Error` | Silently skipped unknowns | Now correctly throws (bug fix) |

0 commit comments

Comments
 (0)