Skip to content

Commit 4a3b830

Browse files
committed
fix(units)!: prefer input-family unit for non-integer scaled values
BREAKING CHANGE: `scaleBy` and `scaleTo` now emit `FractionValue` instead of `DecimalValue` for scaled quantities whose result can be expressed as an allowed fraction on a fraction-enabled unit (e.g. 0.5 on `cup` → `{type:"fraction", num:1, den:2}`). This affects units that have `fractions.enabled` set (cup, lb, oz, tsp, tbsp, fl-oz, etc.). Units without fraction support (g, ml, kg, l, piece, …) are unaffected. Callers that branch on `value.type` or access `value.decimal` directly for these units must handle `FractionValue` alongside `DecimalValue`.
1 parent f5f5f2d commit 4a3b830

5 files changed

Lines changed: 179 additions & 24 deletions

File tree

src/quantities/mutations.ts

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -703,13 +703,21 @@ export function applyBestUnit(
703703

704704
// Get canonical name of the original unit for comparison
705705
const originalCanonicalName = normalizeUnit(extended.unit.name)?.name;
706-
707-
// If same unit (by canonical name match), no change needed - preserve original unit name
708-
if (bestUnit.name === originalCanonicalName) {
709-
return q;
710-
}
711-
712-
// Format the value for the best unit
706+
const isSameUnit = bestUnit.name === originalCanonicalName;
707+
708+
// Determine the output unit representation:
709+
// - When upgrading/downgrading to a different unit, use the canonical best-unit name.
710+
// - When keeping the same unit, preserve the original alias (e.g. "cups" stays "cups").
711+
const outputUnit = isSameUnit
712+
? q.unit
713+
: typeof q.unit === "string"
714+
? bestUnit.name
715+
: { name: bestUnit.name };
716+
717+
// Format the value for the best unit (applies fraction representation when the unit supports
718+
// it, e.g. 0.5 cup → {type:"fraction", num:1, den:2}).
719+
// This is done even when the unit hasn't changed, because the scaling factor may have
720+
// produced a decimal that can now be expressed as a nicer fraction (e.g. 0.5 → 1/2).
713721
const formattedValue = formatOutputValue(bestValue, bestUnit);
714722

715723
// Handle ranges: scale to the best unit
@@ -726,8 +734,7 @@ export function applyBestUnit(
726734
min: formatOutputValue(minValue, bestUnit),
727735
max: formatOutputValue(maxValue, bestUnit),
728736
},
729-
unit:
730-
typeof q.unit === "string" ? bestUnit.name : { name: bestUnit.name },
737+
unit: outputUnit,
731738
} as QuantityWithExtendedUnit | QuantityWithPlainUnit;
732739
}
733740

@@ -737,7 +744,7 @@ export function applyBestUnit(
737744
type: "fixed",
738745
value: formattedValue,
739746
},
740-
unit: typeof q.unit === "string" ? bestUnit.name : { name: bestUnit.name },
747+
unit: outputUnit,
741748
} as QuantityWithExtendedUnit | QuantityWithPlainUnit;
742749
}
743750

src/units/conversion.ts

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -122,22 +122,28 @@ export function findBestUnit(
122122
return integersInInputFamily.sort((a, b) => a.value - b.value)[0]!;
123123
}
124124

125-
// Second priority: integers in any family (prefer system-appropriate units)
125+
// Second priority: non-integers in input family (prefer keeping user's unit even if fractional).
126+
// !isCloseToInteger is implicit here: any input-family integer would have been caught by
127+
// priority 1 above, so the candidates reaching here are guaranteed non-integer.
128+
// Sorted descending by value so we pick the largest (most natural) fraction.
129+
const nonIntegersInInputFamily = inRange.filter((c) =>
130+
inputUnitNames.has(c.unit.name),
131+
);
132+
if (nonIntegersInInputFamily.length > 0) {
133+
return nonIntegersInInputFamily.sort((a, b) => b.value - a.value)[0]!;
134+
}
135+
136+
// Third priority: integers in any family (prefer system-appropriate units)
126137
const integersAny = inRange.filter((c) => isCloseToInteger(c.value));
127138
if (integersAny.length > 0) {
128139
// Sort by value
129140
return integersAny.sort((a, b) => a.value - b.value)[0]!;
130141
}
131142

132-
// Third priority: smallest value in range (prioritizing input family)
133-
return inRange.sort((a, b) => {
134-
// Prioritize input family
135-
const aInFamily = inputUnitNames.has(a.unit.name) ? 0 : 1;
136-
const bInFamily = inputUnitNames.has(b.unit.name) ? 0 : 1;
137-
if (aInFamily !== bInFamily) return aInFamily - bInFamily;
138-
// Then by smallest value
139-
return a.value - b.value;
140-
})[0]!;
143+
// Fourth priority: smallest non-integer value in range
144+
// At this point, all inRange candidates are guaranteed to be non-input-family (since any
145+
// input-family unit in range would have been caught by priority 1 or 2 above).
146+
return inRange.sort((a, b) => a.value - b.value)[0]!;
141147
}
142148

143149
return candidatesWithValues.sort((a, b) => {

test/quantities_mutations.test.ts

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1371,8 +1371,12 @@ describe("applyBestUnit", () => {
13711371
unit: { name: "mL" },
13721372
};
13731373
const result = applyBestUnit(q);
1374-
expect(result).toBe(q);
1374+
// Same unit; value is a whole number so stays decimal (mL has no fractions)
13751375
expect(result.unit?.name).toBe("mL");
1376+
expect(result.quantity).toEqual({
1377+
type: "fixed",
1378+
value: { type: "decimal", decimal: 100 },
1379+
});
13761380
});
13771381

13781382
it("should preserve unit alias when best unit is the same (cups stays cups)", () => {
@@ -1381,8 +1385,12 @@ describe("applyBestUnit", () => {
13811385
unit: { name: "cups" },
13821386
};
13831387
const result = applyBestUnit(q);
1384-
expect(result).toBe(q);
1388+
// Same unit; 2 is a whole number so stays decimal
13851389
expect(result.unit?.name).toBe("cups");
1390+
expect(result.quantity).toEqual({
1391+
type: "fixed",
1392+
value: { type: "decimal", decimal: 2 },
1393+
});
13861394
});
13871395

13881396
it("should infer metric system from unit when system not provided", () => {
@@ -1455,6 +1463,25 @@ describe("applyBestUnit", () => {
14551463
max: { type: "decimal", decimal: 1.5 },
14561464
});
14571465
});
1466+
1467+
it("should apply fraction formatting to range values when unit stays the same", () => {
1468+
// 0.5-1 cup stays as cup but min should become fraction 1/2
1469+
const q: QuantityWithExtendedUnit = {
1470+
quantity: {
1471+
type: "range",
1472+
min: { type: "decimal", decimal: 0.5 },
1473+
max: { type: "decimal", decimal: 1 },
1474+
},
1475+
unit: { name: "cup" },
1476+
};
1477+
const result = applyBestUnit(q);
1478+
expect(result.unit).toEqual({ name: "cup" });
1479+
expect(result.quantity).toEqual({
1480+
type: "range",
1481+
min: { type: "fraction", num: 1, den: 2 },
1482+
max: { type: "decimal", decimal: 1 },
1483+
});
1484+
});
14581485
});
14591486

14601487
describe("subtractQuantities", () => {

test/recipe_scaling.test.ts

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -850,4 +850,78 @@ Add @eggs{5%piece}.
850850
unit: { name: "piece" },
851851
});
852852
});
853+
854+
it("should keep 1/2 cup after round-trip scale ×2 then ×0.5 (not convert to 4 fl-oz)", () => {
855+
// @cream{1/2%cup} → ×2 = 1 cup ✓ → ×0.5 should return to 1/2 cup, NOT 4 fl-oz
856+
const recipe = new Recipe(`
857+
---
858+
servings: 1
859+
---
860+
Add @cream{1/2%cup}.
861+
`);
862+
const step = (r: Recipe) =>
863+
(r.sections[0]!.content[0]! as Step).items.find(
864+
(i) => i.type === "ingredient",
865+
) as IngredientItem;
866+
867+
const doubled = recipe.scaleBy(2);
868+
expect(step(doubled).alternatives[0]).toMatchObject({
869+
quantity: { type: "fixed", value: { type: "decimal", decimal: 1 } },
870+
unit: { name: "cup" },
871+
});
872+
873+
const roundTrip = doubled.scaleBy(0.5);
874+
// 0.5 cup is an allowed fraction (1/2), so it's returned as FractionValue
875+
expect(step(roundTrip).alternatives[0]).toMatchObject({
876+
quantity: { type: "fixed", value: { type: "fraction", num: 1, den: 2 } },
877+
unit: { name: "cup" },
878+
});
879+
});
880+
881+
it("should keep 1/2 lb after round-trip scale ×2 then ×0.5 (not convert to 8 oz)", () => {
882+
const recipe = new Recipe(`
883+
---
884+
servings: 1
885+
---
886+
Add @butter{1/2%lb}.
887+
`);
888+
const step = (r: Recipe) =>
889+
(r.sections[0]!.content[0]! as Step).items.find(
890+
(i) => i.type === "ingredient",
891+
) as IngredientItem;
892+
893+
const doubled = recipe.scaleBy(2);
894+
expect(step(doubled).alternatives[0]).toMatchObject({
895+
quantity: { type: "fixed", value: { type: "decimal", decimal: 1 } },
896+
unit: { name: "lb" },
897+
});
898+
899+
const roundTrip = doubled.scaleBy(0.5);
900+
// 0.5 lb is an allowed fraction (1/2), so it's returned as FractionValue
901+
expect(step(roundTrip).alternatives[0]).toMatchObject({
902+
quantity: { type: "fixed", value: { type: "fraction", num: 1, den: 2 } },
903+
unit: { name: "lb" },
904+
});
905+
});
906+
907+
it("should keep 1.5 cups as cups rather than converting to 12 fl-oz", () => {
908+
// 1 cup × 1.5 = 1.5 cups: non-integer in input family should beat integer in another family
909+
const recipe = new Recipe(`
910+
---
911+
servings: 2
912+
---
913+
Add @cream{1%cup}.
914+
`);
915+
const step = (r: Recipe) =>
916+
(r.sections[0]!.content[0]! as Step).items.find(
917+
(i) => i.type === "ingredient",
918+
) as IngredientItem;
919+
920+
const scaled = recipe.scaleTo(3); // 1 cup for 2 servings → 1.5 cups for 3 servings
921+
// 1.5 cup = 3/2, which is an allowed fraction of cup, so it's returned as FractionValue
922+
expect(step(scaled).alternatives[0]).toMatchObject({
923+
quantity: { type: "fixed", value: { type: "fraction", num: 3, den: 2 } },
924+
unit: { name: "cup" },
925+
});
926+
});
853927
});

test/units_conversion.test.ts

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,9 @@ describe("findBestUnit", () => {
5858
expect(result.value).toBe(240);
5959
});
6060

61-
it("should prefer the smallest integer of all integers in any family over non-integers in input family", () => {
62-
// 236.6 ml = ~1 cup or ~8 fl-oz
61+
it("should prefer the smallest integer of all integers in any family over non-integers in input family (when input family unit has no fractions)", () => {
62+
// 236.6 ml in US system: mL is metric (not US-compatible), so excluded from candidates.
63+
// cup ≈ 1 (integer, any family) wins. This is the system-conversion case.
6364
const mLDef = normalizeUnit("mL")!;
6465
const result = findBestUnit(236.6, "volume", "US", [mLDef]);
6566
expect(result.unit.name).toBe("cup");
@@ -136,4 +137,44 @@ describe("findBestUnit", () => {
136137
expect(result.unit.name).toBe("tsp");
137138
expect(result.value).toBeCloseTo(0.06, 2);
138139
});
140+
141+
it("should prefer non-integer value in input family over integer in another family (Option B)", () => {
142+
// 118.294 ml = 0.5 cup (fraction, input family) vs 4 fl-oz (integer, other family)
143+
// Priority 2: non-integer in input family wins → cup
144+
const cupDef = normalizeUnit("cup")!;
145+
const result = findBestUnit(118.294, "volume", "US", [cupDef]);
146+
expect(result.unit.name).toBe("cup");
147+
expect(result.value).toBeCloseTo(0.5);
148+
149+
// 0.5 lb (input family) vs 8 oz (integer, other family)
150+
const lbDef = normalizeUnit("lb")!;
151+
const result2 = findBestUnit(226.796, "mass", "US", [lbDef]);
152+
expect(result2.unit.name).toBe("lb");
153+
expect(result2.value).toBeCloseTo(0.5);
154+
155+
// 1.5 cups (input family, non-integer) vs 12 fl-oz (integer, other family)
156+
const result3 = findBestUnit(354.882, "volume", "US", [cupDef]);
157+
expect(result3.unit.name).toBe("cup");
158+
expect(result3.value).toBeCloseTo(1.5);
159+
});
160+
161+
it("should sort multiple non-integer input-family candidates by largest value first", () => {
162+
// 350ml in US, input=[cup, fl-oz]: cup≈1.479, fl-oz≈11.834 — both non-integer, both in range
163+
// Sort descending by value → fl-oz wins (exercises the sort comparator when 2+ candidates)
164+
const cupDef = normalizeUnit("cup")!;
165+
const flozDef = normalizeUnit("fl-oz")!;
166+
const result = findBestUnit(350, "volume", "US", [cupDef, flozDef]);
167+
expect(result.unit.name).toBe("fl-oz");
168+
expect(result.value).toBeCloseTo(11.83, 1);
169+
});
170+
171+
it("should pick smallest non-integer in range when input unit is out of range", () => {
172+
// 30ml in US with tsp as input: tsp≈6.09 is above maxValue(5) so out of range.
173+
// fl-oz≈1.015 and tbsp≈2.03 are both non-integer and in range but not in input family.
174+
// Fourth priority picks smallest: fl-oz=1.015 < tbsp=2.03
175+
const tspDef = normalizeUnit("tsp")!;
176+
const result = findBestUnit(30, "volume", "US", [tspDef]);
177+
expect(result.unit.name).toBe("fl-oz");
178+
expect(result.value).toBeCloseTo(1.015, 2);
179+
});
139180
});

0 commit comments

Comments
 (0)