Skip to content

fix: disable bundled_units so quantities keep their authored units - #433

Merged
dubadub merged 1 commit into
mainfrom
fix/disable-bundled-units
Aug 13, 2026
Merged

fix: disable bundled_units so quantities keep their authored units#433
dubadub merged 1 commit into
mainfrom
fix/disable-bundled-units

Conversation

@dubadub

@dubadub dubadub commented Aug 13, 2026

Copy link
Copy Markdown
Member

Summary

Disables the cooklang bundled_units feature so quantities are displayed exactly as authored, in the units the recipe author wrote.

bundled_units loads the full unit database into Converter::default(), which:

  • silently converted quantities to a "best" unit (5/8 cup10 tbsp),
  • approximated fractions lossily (1 5/81 2/3, a ~2.6% error presented as exact),
  • made the terminal and web UI disagree on the same recipe (web showed 1.625 c, terminal 1 2/3 c).

Root cause

No dependency leaks the feature anymore — cooklang-reports and cooklang-language-server both declare cooklang with default-features = false. It was enabled solely by our own explicit declaration in Cargo.toml, guarded by a stale comment about past feature unification. The comment now documents why the feature must stay off and how to find a future leak (cargo tree -e features -i cooklang).

Behavior after

With the feature off, Converter::default() falls back to Converter::empty() (no code changes needed):

$ cook recipe units-test.cook   # @milk{1 5/8%cup}, @cocoa{5/8%cup}

Ingredients:
  cocoa      0.625 cup
  milk       1.625 cup

Authored units preserved, exact values, full unit names, and terminal/web UI now agree.

Snapshot changes worth noting

  • scalable flips falsetrue: the old conversion path rebuilt quantities via Quantity::new, which hardcodes scalable: false, silently dropping the parser's scalable flag. The new value is the correct one.
  • The scaled-recipe snapshot loses a floating-point error term (err: -6.76e-8) left over from the lossy 3 tsp1 tbsp round-trip.

Addresses the silent-conversion and web/terminal-consistency parts of #432. The remaining part (opt-in fraction display, e.g. 1 5/8 cups) stays open there.

Checks

  • cargo fmt --check, cargo clippy, and the full test suite pass.

The cooklang bundled_units feature loads the full unit database into
Converter::default(), which silently converts quantities to a "best"
unit (5/8 cup -> 10 tbsp) and approximates fractions lossily
(1 5/8 -> 1 2/3, a ~2.6% error shown as if exact). It also made the
terminal and web UI disagree on the same recipe.

No dependency leaks it anymore: cooklang-reports and
cooklang-language-server both declare default-features = false. The
feature was only enabled by our own explicit declaration, based on a
stale comment about past feature unification.

With the feature off, Converter::default() is empty, so quantities stay
exactly as authored in every output format, and both UIs agree.

Snapshot changes beyond units: `scalable` flips false -> true because
the conversion path rebuilt quantities via Quantity::new (which
hardcodes scalable: false), silently dropping the parser's scalable
flag; and the scaled-recipe snapshot loses a floating-point error term
left over from the lossy 3 tsp -> 1 tbsp round-trip.

Addresses the unit-conversion and consistency parts of #432.
@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review

Small, well-scoped change with a clear root-cause writeup. A few notes:

Code quality

  • Good catch tracking this down to cooklang-reports/cooklang-language-server no longer leaking the feature, and leaving a comment (cargo tree -e features -i cooklang) so a future re-leak is discoverable instead of silently reintroducing lossy conversion. That's exactly the kind of "why" comment that earns its keep.
  • No source changes needed, which keeps the risk surface small — this is purely a dependency-feature flip plus regenerated snapshots/lockfile.

Behavior / potential issue worth flagging

  • PARSER (src/util/mod.rs:50-53) is a single shared CooklangParser::new(Extensions::empty(), Converter::default()) used by both cook recipe output and cook shopping_list (src/shopping_list.rs:300,307,311) for ingredient aggregation across multiple recipes. With bundled_units off, Converter::default() == Converter::empty(), so it's not just display formatting that changes — shopping-list merging of the same ingredient specified in different-but-compatible units across recipes (e.g. 2 cups in one recipe, 500 ml in another) will no longer be unified, since there's no unit database to convert through. That may be an acceptable tradeoff given the accuracy issues described, but it's a functional behavior change beyond "quantities keep their authored units" in cook recipe, and isn't mentioned in the PR description. Worth confirming this is intended, and maybe worth a manual cook shopping_list check with mixed-unit recipes before merging, since the "Testing Approach" in CLAUDE.md notes there's no automated coverage here.

Test coverage

  • Snapshot updates look consistent with the described change (units preserved, scalable: true now correct per the parser's flag instead of being hardcoded false by the old Quantity::new rebuild path, and the stray floating-point error term from the lossy tsptbsp round trip is gone). No new test was added to explicitly assert "units are preserved as authored" going forward — given this is the entire point of the PR, a small regression test (even just a snapshot on a fixture with 5/8 cup style quantities) would help prevent silent reintroduction if the feature default ever flips upstream.

Security / performance

  • No concerns. Dropping bundled_units actually trims the dependency graph slightly (removes prettyplease/syn/proc-macro2/quote from the lockfile), which is a nice side benefit for build times.

Overall this looks correct and matches the stated goal. The shopping-list aggregation implication is the one thing I'd want explicitly confirmed as intended before merging.

@dubadub
dubadub merged commit af80644 into main Aug 13, 2026
5 of 6 checks passed
@dubadub
dubadub deleted the fix/disable-bundled-units branch August 13, 2026 15:11
dubadub added a commit that referenced this pull request Aug 13, 2026
`cooklang::quantity::GroupedQuantity` keeps the quantities it could not add
together in a `HashMap` keyed by unit name, and Rust randomises `HashMap`
iteration per process. Anything rendering a group in its own order therefore
printed a different order on every run:

    flour 1 cup, 100 g
    flour 100 g, 1 cup

This only became visible with #433, which removed the `bundled_units` feature
so quantities keep their authored units: with no unit database no unit is
"known", so every component lands in that map and any ingredient measured two
ways came out in random order. This is the fourth time `HashMap` iteration
order has reached user-visible output here (see #427).

`format::quantity::ordered_components` fixes one rule — components are ordered
by unit name, the unitless one first, components sharing a unit keeping the
order they were added — and every site that renders or serialises a group now
goes through it: the shopping list's human table, Markdown, JSON and YAML
writers and `AggregatedList`'s own rendered strings, the recipe human, Markdown,
LaTeX, Typst and schema.org formatters, and the web UI's ingredient lists and
the `grouped_ingredients` array of the recipe API. The order the units were
written in is not recoverable — the map has already lost it — so unit name is
what is left that is deterministic, identical on every platform, and short
enough to document in a sentence.

`crates/core/Cargo.toml` drops `bundled_units` here too, which is what surfaced
the bug: enabling it in core would switch it back on for `cookcli` through
feature unification and undo #433. The core tests it invalidates are updated
with it — `ml` and `l` no longer add up, `1 c` is now `1 cup`, and `scale(1.0)`
has no database left to re-fit against.
dubadub added a commit that referenced this pull request Aug 13, 2026
…dled_units

These ten were recorded while `cooklang`'s unit database was compiled in. With
it gone (#433) quantities keep the units they were authored in, so the recorded
output legitimately changes:

    1500 ml  ->  1 l, 500 ml   ml and l no longer add up
    1 c      ->  1 cup         no database to abbreviate against
    1.5 l    ->  1500 ml       scaling no longer re-fits
    1 tbsp   ->  3 tsp         likewise

Re-recorded only after the preceding commit made the output stable: the
generated snapshots were checked byte-identical across six consecutive runs
first, because without that fix the components of `1 l, 500 ml` and
`1 cup, 200 g` come out of a `HashMap` and five to eight of these fourteen
tests fail at random on any given run.

Accepted with `INSTA_UPDATE=always cargo test --test
shopping_list_characterization_test`, scoped to this one target so no other
golden file could be rewritten; #433's own recipe snapshots are untouched.
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