Skip to content

fix: don't treat Windows drive-letter colons as scaling delimiter#338

Merged
dubadub merged 1 commit into
mainfrom
fix/windows-absolute-paths-335
May 15, 2026
Merged

fix: don't treat Windows drive-letter colons as scaling delimiter#338
dubadub merged 1 commit into
mainfrom
fix/windows-absolute-paths-335

Conversation

@dubadub
Copy link
Copy Markdown
Member

@dubadub dubadub commented May 14, 2026

Summary

  • Fixes Windows: Absolute paths throw error #335. On Windows, paths like C:\test\recipe.cook were being split on the drive-letter colon, producing the error Invalid scaling factor for 'C': invalid float literal.
  • split_recipe_name_and_scaling_factor now only splits when the right side actually parses as f64, so drive-letter paths fall through unchanged. Returning Option<(&str, f64)> also let me drop the parse-and-abort logic in all three callers (recipe read, report, extract_ingredients).

Behavioral note

A typo like recipe.cook:abc no longer surfaces as Invalid scaling factor; it now flows through and reports Recipe not found: recipe.cook:abc. I think that's clearer (the user gave a path; we report it doesn't exist), but happy to add an explicit "looks like a typo'd scaling factor" check if reviewers prefer.

Test plan

  • cargo fmt --check
  • cargo clippy --all-targets -- -D warnings
  • cargo test (all existing tests + 5 new unit tests for the helper)
  • Manual smoke: cook recipe read seed/Breakfast/Easy\ Pancakes.cook:2 still scales correctly
  • Manual smoke: cook recipe read 'C:\test\recipe.cook' now reports "Recipe not found" instead of the float-parse error

The recipe-path parser used rsplit_once(':') to separate an optional
scaling factor, which broke Windows absolute paths like
C:\test\recipe.cook (the path was split into name "C" and factor
"\test\recipe.cook", failing as an invalid float).

Now the helper only splits when the right side parses as f64, so
drive-letter paths fall through unchanged.

Closes #335
@claude
Copy link
Copy Markdown

claude Bot commented May 14, 2026

Code Review

This is a clean, well-scoped fix. The core approach — parse f64 inside the helper and return None on failure — is exactly right for this bug. Here's my full assessment.

What's Good

  • Root cause fix is correct. Moving the f64 parse into split_recipe_name_and_scaling_factor and returning None on failure is the right place to handle this. A drive-letter colon (C:) naturally falls through because \test\recipe.cook won't parse as a float.
  • Idiomatic Rust. The rewrite to rsplit_once + ? + .ok()? is clean and readable. Eliminating the duplicate parse-and-abort boilerplate from all three callers is a genuine improvement.
  • Tests are good. All five cases are meaningful: integer scale, decimal scale, no colon, Windows path regression, and non-numeric right side. The regression test is especially valuable for preventing future regressions.
  • Import cleanup. Removing now-unused CommandFactory and clap::CommandFactory imports is tidy.

Concerns

1. UX regression for typos (acknowledged in PR, but worth deciding)

The original code gave a precise error for recipe.cook:2x:

Invalid scaling target for 'recipe.cook': invalid float literal. Use a number value after : to specify a scaling factor.

The new behavior treats it as a path and reports:

Recipe not found: recipe.cook:2x

That's genuinely confusing for a user who intended to scale. A lightweight heuristic could catch this in the callers — if split_recipe_name_and_scaling_factor returns None but the query contains : followed by something that starts with a digit (or -), surface a warning. That's a small addition and doesn't affect the Windows fix at all. Not blocking, but I'd lean toward adding it.

2. Missing test: Windows path with valid scaling

C:\test\recipe.cook:2 should correctly return Some(("C:\\test\\recipe.cook", 2.0)) because rsplit_once splits on the last colon. Worth adding a test to lock this in, since it's the main happy-path scenario on Windows:

#[test]
fn splits_windows_path_with_scaling_factor() {
    assert_eq!(
        split_recipe_name_and_scaling_factor(r"C:\test\recipe.cook:2"),
        Some((r"C:\test\recipe.cook", 2.0))
    );
}

3. Negative and zero scaling

recipe.cook:0Some(("recipe.cook", 0.0)) and recipe.cook:-2Some(("recipe.cook", -2.0)) both parse fine. If the cooklang crate rejects those values, the error will appear later in the pipeline (probably fine). Just confirming there's no silent data loss here — there isn't.

Summary

The fix is correct and the code quality is high. Two things I'd recommend before merging:

  1. Add the Windows-path-with-scaling test case.
  2. Make a call on the typo UX — either add the heuristic or document the behavior change in CHANGELOG/release notes so it doesn't surprise users.

@dubadub dubadub merged commit 4bc724d into main May 15, 2026
6 checks passed
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.

Windows: Absolute paths throw error

1 participant