fix(sample_source_call): respect local argument (#9)

[VincentGuyader]

Summary

• sample_source_call() was always returning the French-locale list (Local, France, Europe, International) regardless of the local = argument, leaking France into datasets generated with local = "en_US". Branched on the locale so en_US now yields Local, US, Europe, International.
• local = is now declared with match.arg(), matching the rest of the file's contract: any unsupported locale (e.g. "de_DE") errors out instead of silently falling through to en_US.

Test plan

• tests/testthat/test-locale.R: 6 PASS, including an explicit assertion that France does not appear in the en_US factor levels and a regression on the unknown-locale rejection.

Closes #9

[ColinFay]

Review

The core fix is correct: I checked out the branch and confirmed fake_ticket_client(local = "en_US")$source_call no longer leaks France, fr_FR output is unchanged, and the full test suite passes.

Most of the remarks below are about the new test file, which I think currently gives less protection than it looks like it does. Blocking items first.

Tests

1. tests/testthat/test-locale.R:18 — the test fails on a non-English R installation.

It matches base R's match.arg() message, which is translated. Reproduced with:

LANGUAGE=fr LC_ALL=fr_FR.UTF-8 Rscript -e 'pkgload::load_all(); testthat::test_file("tests/testthat/test-locale.R")'
# Failure (test-locale.R:16): expected match "should be one of"
# actual message: 'arg' doit être un de "en_US", "fr_FR"

DESCRIPTION has no Config/testthat/edition: 3, so we're on edition 2 and local_reproducible_output() (which would pin LANGUAGE="en") never runs. This hits anyone running devtools::test() in a French locale, and CRAN's translated-locale checks. Either withr::local_language("en") in the test, or assert on the condition class instead of the message.

2. tests/testthat/test-locale.R:11 — the fr_FR assertion is vacuous.

"Europe" is in both level vectors (c("Local","France","Europe","International") and c("Local","US","Europe","International")), so the || "Europe" %in% ... disjunct is always satisfied. I checked it against a deliberately broken result — factor(sample(c("Local","US","Europe","International"), 200, TRUE)) — and the assertion still returns TRUE. This is the only test guarding the French branch through fake_ticket_client, and inverting the new if (local == "fr_FR") branch would leave it green. Suggest expect_true("France" %in% levels(df$source_appel)).

3. tests/testthat/test-locale.R:27 — the PR's central claim isn't tested.

Every new assertion checks the absence of "France"; nothing checks that en_US actually yields "US". If the vector were mistyped as c("Local", "USA", "Europe", "International") or lost a level, all six new assertions would still pass. Worth adding expect_true("US" %in% levels(en)).

4. tests/testthat/test-locale.R:23 — dead guard.

skip_if_not(exists("sample_source_call", envir = asNamespace("fakir"))) can never skip, since the function is unconditionally defined in R/utils.R — and the test just above already calls fakir:::sample_source_call with no guard. Its only possible effect is turning a real "function was deleted" regression into a silent skip.

5. tests/testthat/test-locale.R:24 — the with_seed() wrapper buys nothing.

Lines 31–32 only inspect levels(), which is the fixed source_level vector and independent of the RNG, so the seeded block and the 200-element samples aren't doing any work. Similarly, line 6 is implied by line 5 (a factor's values are a subset of its levels), and the || "France" %in% as.character(fr) disjunct on line 32 is implied by its own left-hand side. The file reduces to two levels() assertions on sample_source_call(1, local = ...).

Implementation

6. R/utils.R:86 — the else branch reintroduces the bug this PR fixes.

With en_US handled by else, adding a locale to the signature later (say local = c("en_US", "fr_FR", "de_DE")) makes match.arg() accept it and silently return US labels for a German dataset. Every sibling helper in this file (sample_state_level, sample_yes, sample_type) uses two explicit if (local == ...) branches and fails loudly on an unhandled locale. An explicit if (local == "en_US") keeps that property.

7. R/utils.R:84 — the level vectors are now duplicated.

sample_source (R/utils.R:111-115) already holds the identical two vectors, and is covered by tests/testthat/test-utils.R:124-147. This makes a third and fourth copy, so the two helpers can drift independently — which is more or less how #9 happened. Extracting a source_levels(local) helper and calling it from both would close the class of bug rather than this instance.

8. R/utils.R:83 — match.arg() is added to 1 of the 9 locale helpers.

On the branch:

recode_sample_yes(c("Yes","No"), "de_DE")  # NULL, silently -> all-NA column
sample_yes(3, "de_DE")                     # Error: object 'vec' not found

and recode_sample_types, sample_type, sample_state, sample_state_level, sample_priority, sample_source all behave the same way. The PR body says this "matches the rest of the file's contract", but it currently makes sample_source_call the sole outlier. Either one shared check_local() across the helpers, or rely on the match.arg already present in the exported entry points.

9. R/utils.R:83 — match.arg() isn't the strict check the PR body describes.

It does partial matching and accepts NULL:

sample_source_call(5, local = "fr")    # levels: Local France Europe International — no error
sample_source_call(5, local = NULL)    # en_US levels — no error

So "any unsupported locale errors out instead of silently falling through to en_US" isn't quite true: NULL still falls through, and any unambiguous prefix is accepted. If strict rejection is the goal, stop_if_not() (already imported from attempt in R/fake_client.R:11) would do it.

Outside the fix

10. R/utils.R:95 — sample_type is defined twice in the touched file.

Lines 27–40 define it with prob = runif(3, 0.25, 1); lines 95–99 redefine it without prob. R evaluates top to bottom, so the second wins — body(fakir:::sample_type) on the loaded package shows the unweighted version. Lines 27–40 are dead code, and the roxygen @details in R/fake_client.R:161 ("Some types are more sampled than others") is currently false: type is sampled uniformly. Pre-existing, two lines below this hunk — worth a separate issue.

11. NEWS.md — no entry for a user-visible change.

DESCRIPTION is at Version: 1.1.0 but NEWS.md's top section is # fakir 1.0.0, and it already carries * fix: fixing issue with locals in fake_ticket_client (#9) — written before the fix existed. After this PR, fake_ticket_client(local = "en_US")$source_call changes levels from Local/France/Europe/International to Local/US/Europe/International. Anyone with teaching material, a vignette, a pinned snapshot or a filter(source_call == "France") on an en_US dataset breaks with nothing in the changelog. A # fakir 1.1.0 section describing the level change would cover it.

12. .github/workflows/pkgdown.yaml — unrelated CI bump bundled in.

Commit 0c11133 bumps actions/checkout v2→v4 and three r-lib/actions v1→v2. The change is fine on its own (R-CMD-check.yaml, pr-commands.yaml and test-coverage.yaml on master are already at those versions, so it only restores consistency), but it makes the bugfix non-revertible in isolation. Better as its own PR.