Skip to content

Dedupe gsub helper and drop redundant relevel_taxonomic_status_preferred_order() - #293

Open
ehwenk wants to merge 5 commits into
masterfrom
fix/282-code-cleanups
Open

Dedupe gsub helper and drop redundant relevel_taxonomic_status_preferred_order()#293
ehwenk wants to merge 5 commits into
masterfrom
fix/282-code-cleanups

Conversation

@ehwenk

@ehwenk ehwenk commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the remaining part of #282: promotes the identical gsub-wrapper helper duplicated in standardise_names(), strip_names(), and strip_names_extra() into a single shared helper. Also removes an unrelated exact-duplicate function found while reviewing the same area.

What changed

  1. Dedupe the gsub-perl helper (Minor code cleanups: duplicated helpers and per-row fuzzy loops #282). All three functions defined an identical local helper. Promoted to a single gsub_perl() in new R/utils.R. standardise_taxon_rank()'s local helper is fixed = TRUE, not perl = TRUE, so it stays local — renamed gsub_fixed() so the two regex semantics are told apart by name rather than by scope.

  2. Drop redundant nested relevel_taxonomic_status_preferred_order(). synonyms_for_accepted_names() re-defined a byte-identical copy of this function, which already exists at top level (and is thus package-wide visible) in update_taxonomy.R — the duplicate's own comment even said (function from update_taxonomy.R). Deleted the nested copy; the call site now resolves to the shared top-level function.

Not in scope here

The other two cleanups originally listed in #282 — the repeated slash-pattern predicate and the per-row fuzzy-match loops in match_taxa() — were already resolved by #283's refactor (apply_match()/fuzzy_match_column()/is_indecision()). No further change needed there.

Verification

Behaviour-preserving: outputs of all four touched string functions are identical() to master, and so is synonyms_for_accepted_names(). devtools::test() — 180 passed, 0 failed. devtools::document() — no NAMESPACE/man/ changes.

Closes #282

🤖 Generated with Claude Code

ehwenk and others added 2 commits August 20, 2026 12:24
standardise_names(), strip_names(), and strip_names_extra() each defined
an identical local helper:

  f <- function(x, find, replace) gsub(find, replace, x, perl = TRUE)

Promote it to a single internal helper in a new R/utils.R and drop the
three local copies; call sites are unchanged since the shared helper
keeps the name `f`.

standardise_taxon_rank()'s local `f` (fixed = TRUE, not perl = TRUE) is
left alone -- it isn't identical to the other three.

The other two cleanups in #282 (the repeated slash-pattern predicate and
the per-row fuzzy-match loops in match_taxa()) were already resolved by
the #283 refactor (apply_match()/fuzzy_match_column()/is_indecision()),
confirmed via `git log -- R/match_taxa.R` and the "match_taxa.R
duplication" note in AGENTS.md -- no further change needed there.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
synonyms_for_accepted_names() re-defined a byte-identical copy of
relevel_taxonomic_status_preferred_order(), already a top-level (and
thus package-wide visible) function in update_taxonomy.R -- the
duplicate's own comment even said "(function from update_taxonomy.R)".
Delete the nested copy; the call site now resolves to the shared
top-level function.

Found while reviewing #282; not part of that issue's original scope
but the same kind of exact-duplicate-helper cleanup.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ehwenk
ehwenk requested a review from dfalster August 20, 2026 04:42
`f` at package scope is ungreppable from its ~60 call sites, and it was
shadowed in the same file by `standardise_taxon_rank()`'s local `f`
(`fixed = TRUE`, not `perl = TRUE`) — two different regex semantics
behind one name, one visible and one not.

Rename to `gsub_perl()` (shared, in utils.R) and `gsub_fixed()` (local).
Also drops two blank lines left after the opening brace in strip_names.R.

Verified: outputs of standardise_names(), strip_names(),
strip_names_extra() and standardise_taxon_rank() are identical() to
master over 33 adversarial names + 13 ranks; devtools::test() 180 pass,
0 fail; devtools::document() leaves man/ and NAMESPACE unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dfalster

Copy link
Copy Markdown
Member

Reviewed, and I've pushed one follow-up commit (17f405c) rather than leaving it as a request — details below. Both original changes check out; I verified them rather than taking the description's word for it.

Verification

Both changes are genuinely behaviour-preserving. I loaded worktrees at origin/master (406e156) and the PR head with pkgload::load_all() and diffed the outputs:

  • 33 adversarial inputs (phrase names, s.l./s.str., forma/f., affinis vs aff., ×, slashes, tabs, double dots, unbalanced parens, NA, "") through standardise_names(), strip_names(), strip_names_extra(), plus 13 through standardise_taxon_rank()identical().
  • synonyms_for_accepted_names() on 8 accepted names, both collapse = TRUE and FALSE — 66 synonym rows, enough to actually exercise the taxonomic_status releveling → identical().

The relevel_taxonomic_status_preferred_order() deletion is safe. The surviving definition at R/update_taxonomy.R:263 is at column 0, i.e. genuinely top level and not nested inside update_taxonomy() — and load_taxonomic_resources.R already calls it from a different file, which independently proves package-wide visibility. The bodies differ only in the indentation of the closing factor() call; the 19-element preferred_order vector is character-for-character the same. The deleted nested copy closed over nothing from its enclosing scope, so there's no captured-environment subtlety.

Closes #282 is legitimate. I checked the two items declared out of scope rather than trusting the claim: grep for for (i in seq_len across R/ returns nothing, and is_indecision() is defined once at R/match_taxa.R:116 and used at every match step that needs it. Items 2 and 3 really were absorbed by #283.

What I pushed, and why

The one thing I didn't want to merge as-is was the helper being named f. Keeping the name does make the diff smaller, but:

  • It's ungreppable. A reader of strip_names() now sees f("\\.", "") with no definition in the file and no realistic way to search for where it came from.
  • It set a trap. standardise_taxon_rank() still defined a local f with fixed = TRUE, which shadowed the new package-level perl = TRUE one — two different regex semantics behind one name, in the same file, one visible and one not. There's no live bug today: those seven patterns (regnum, forma, sectio, …) contain no metacharacters and the replacements no backrefs, so fixed and perl agree exactly. But the next person doing precisely what this PR does — "there's a duplicate local f, delete it" — would make a silent semantic change that no test would catch.

So 17f405c renames them to gsub_perl() (shared, in utils.R) and gsub_fixed() (local), which removes the shadowing entirely and makes all ~60 call sites self-documenting. It also drops two blank lines left after the opening brace in strip_names.R, where standardise_names() had none.

Re-verified after the rename: same identical() results against master, devtools::test() 180 passed / 0 failed, and devtools::document() leaves man/ and NAMESPACE untouched (confirming @noRd keeps the helper out of the docs). I also trimmed the "keeping the name f" clause from the PR description, since this repo squash-merges and the body becomes the permanent commit message.

Two notes for the record: f was the only single-letter top-level object in the package, and nothing else in R/ or tests/ uses f as a variable, column, or argument name, so the original version wasn't colliding with anything — the objection was readability, not correctness. And DESCRIPTION has no Collate field, so the new file needed no registration.

Happy for this to go in once CI is green on the new commit.

ehwenk and others added 2 commits August 24, 2026 08:18
#292 added `g()` (str_replace anchored to end-of-string) specifically to
stop "sectio"->"section" and "forma"->"form" corrupting values that
already contain those as a substring ("section" -> "sectionn",
"informal" -> "informl") -- its own comment explains exactly this. But
the last two pipeline steps still called the old unanchored
`gsub_fixed()` instead of the new `g()`, so the bug it was meant to fix
was still live. Switch those two calls to `g()`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.

Minor code cleanups: duplicated helpers and per-row fuzzy loops

2 participants