refactor(migrations): split the registry into per-topic files

.github/workflows/custom/after-install/action.yml

@@ -9,7 +9,8 @@ runs:
         echo -e 'CPPFLAGS = -I/opt/homebrew/include\nLDFLAGS = -L/opt/homebrew/lib' | tee ~/.R/Makevars
       shell: bash
 
-    # Regenerate the in-place ARG_HANDLE blocks from tools/migrations.R and fail
+    # Regenerate the in-place ARG_HANDLE blocks from the registry files under
+    # tools/migrations/ and fail
     # if they have drifted. Runs from the source checkout (R is available after
     # install; tools/ is excluded from the built package) so a contributor who
     # forgot to regenerate gets a hard failure. Local dev refreshes them
@@ -18,7 +19,7 @@ runs:
       run: |
         Rscript tools/generate-migrations.R
         if ! git diff --exit-code -- R; then
-          echo "::error::Generated ARG_HANDLE blocks are out of sync with tools/migrations.R."
+          echo "::error::Generated ARG_HANDLE blocks are out of sync with the registry (tools/migrations/)."
           echo "Run 'Rscript tools/generate-migrations.R' and commit the result."
           exit 1
         fi

tests/testthat/helper-migrations.R

@@ -1,21 +1,27 @@
 # Keep the in-place ARG_HANDLE blocks in sync with the registry during
-# development: regenerate them before the tests run so editing tools/migrations.R
-# and re-running the tests refreshes the spliced code.
+# development: regenerate them before the tests run so editing any registry
+# file under tools/migrations/ and re-running the tests refreshes the
+# spliced code.
 #
 # tools/ is excluded from the built package (.Rbuildignore), so this is a no-op
 # under `R CMD check` of the tarball; it only fires from a source checkout
 # (devtools::test(), covr). The generator only rewrites a file when its block
 # actually changes, so a clean tree stays clean. CI additionally guards against
 # committed drift (see .github/workflows/custom/before-install).
 local({
-  registry <- testthat::test_path("..", "..", "tools", "migrations.R")
   generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R")
-  if (!file.exists(registry) || !file.exists(generator)) {
+  if (!file.exists(generator)) {
     return(invisible())
   }
   src_dir <- testthat::test_path("..", "..", "R")
 
   gen_env <- new.env()
   sys.source(generator, envir = gen_env)
+  registry <- gen_env$migration_registry_files(
+    testthat::test_path("..", "..")
+  )
+  if (length(registry) == 0) {
+    return(invisible())
+  }
   suppressMessages(gen_env$generate_migrations(registry, src_dir))
 })

tests/testthat/test-generate-migrations.R

@@ -0,0 +1,158 @@
+# Unit tests for the registry loader in tools/generate-migrations.R.
+# The generator is plain base R and only exists in a source checkout
+# (tools/ is .Rbuildignore'd), so these tests are skipped in a built package.
+
+local_generator <- function(env = parent.frame()) {
+  generator <- testthat::test_path("..", "..", "tools", "generate-migrations.R")
+  skip_if_not(file.exists(generator), "tools/generate-migrations.R not found")
+  gen_env <- new.env()
+  sys.source(generator, envir = gen_env)
+  gen_env
+}
+
+write_registry <- function(dir, name, code) {
+  path <- file.path(dir, name)
+  writeLines(code, path)
+  path
+}
+
+test_that("migration_registry_files() finds the legacy file and topic files", {
+  gen <- local_generator()
+  root <- withr::local_tempdir()
+  dir.create(file.path(root, "tools", "migrations"), recursive = TRUE)
+
+  expect_identical(gen$migration_registry_files(root), character(0))
+
+  legacy <- write_registry(
+    file.path(root, "tools"),
+    "migrations.R",
+    "migrations <- list()"
+  )
+  b <- write_registry(
+    file.path(root, "tools", "migrations"),
+    "b-topic.R",
+    "migrations <- list()"
+  )
+  a <- write_registry(
+    file.path(root, "tools", "migrations"),
+    "a-topic.R",
+    "migrations <- list()"
+  )
+
+  # legacy first, then topic files sorted by path
+  expect_identical(gen$migration_registry_files(root), c(legacy, a, b))
+})
+
+test_that("load_migrations() merges entries across registry files", {
+  gen <- local_generator()
+  dir <- withr::local_tempdir()
+
+  one <- write_registry(
+    dir,
+    "one.R",
+    c(
+      "migrations <- list(",
+      "  fn_one = list(",
+      "    old = function(x, a) {},",
+      "    new = function(x, ..., a = 1) {},",
+      "    when = '3.0.0'",
+      "  )",
+      ")"
+    )
+  )
+  two <- write_registry(
+    dir,
+    "two.R",
+    c(
+      "migrations <- list(",
+      "  fn_two = list(",
+      "    old = function(y, b) {},",
+      "    new = function(y, ..., b = 2) {},",
+      "    when = '3.0.0'",
+      "  )",
+      ")"
+    )
+  )
+
+  merged <- gen$load_migrations(c(one, two))
+  expect_named(merged, c("fn_one", "fn_two"))
+  expect_identical(merged$fn_two$head, "y")
+})
+
+test_that("load_migrations() rejects duplicate entries across files", {
+  gen <- local_generator()
+  dir <- withr::local_tempdir()
+
+  entry <- c(
+    "migrations <- list(",
+    "  fn_dup = list(",
+    "    old = function(x, a) {},",
+    "    new = function(x, ..., a = 1) {},",
+    "    when = '3.0.0'",
+    "  )",
+    ")"
+  )
+  one <- write_registry(dir, "one.R", entry)
+  two <- write_registry(dir, "two.R", entry)
+
+  expect_error(
+    gen$load_migrations(c(one, two)),
+    "Duplicate migration entry across registry files: fn_dup"
+  )
+})
+
+test_that("load_migrations() rejects files without a migrations list", {
+  gen <- local_generator()
+  dir <- withr::local_tempdir()
+
+  bad <- write_registry(dir, "bad.R", "not_migrations <- list()")
+  expect_error(
+    gen$load_migrations(bad),
+    "must define a `migrations` list"
+  )
+})
+
+test_that("load_migrations() tolerates empty registries", {
+  gen <- local_generator()
+  dir <- withr::local_tempdir()
+
+  empty <- write_registry(dir, "empty.R", "migrations <- list()")
+  expect_identical(gen$load_migrations(empty), list())
+  expect_identical(gen$load_migrations(character(0)), list())
+})
+
+test_that("render_call_arg() wraps a single long item without a stray comma", {
+  gen <- local_generator()
+  long_item <- paste0(
+    "impl = c(\"auto\", \"copy_and_delete\", \"create_from_scratch\")"
+  )
+  out <- gen$render_call_arg("defaults", "list", long_item, "list()")
+  expect_identical(
+    out,
+    c(
+      "    defaults = list(",
+      paste0("      ", long_item),
+      "    ),"
+    )
+  )
+  # and no element is a bare comma
+  expect_false(any(grepl("^\\s*,\\s*$", out)))
+})
+
+test_that("default expressions keep air's spacing around binary `/`", {
+  gen <- local_generator()
+  entry <- list(
+    old = function(graph, agebins, base) {},
+    new = function(
+      graph,
+      ...,
+      agebins = n / 7100,
+      base = "http://a/b"
+    ) {},
+    when = "3.0.0"
+  )
+  norm <- gen$normalise_migration("fn_slash", entry)
+  expect_identical(norm$defaults$agebins, "n / 7100")
+  # slashes inside string literals stay untouched
+  expect_identical(norm$defaults$base, "\"http://a/b\"")
+})

tests/testthat/test-migration-fixture.R

@@ -160,7 +160,9 @@ test_that("the generated block is in sync with the registry", {
 
   gen_env <- new.env()
   sys.source(generator, envir = gen_env)
-  registry <- testthat::test_path("..", "..", "tools", "migrations.R")
+  registry <- gen_env$migration_registry_files(
+    testthat::test_path("..", "..")
+  )
   migrations <- gen_env$load_migrations(registry)
   by_fn <- stats::setNames(
     migrations,

tools/README.md

@@ -156,15 +156,19 @@ soft-deprecation.
 
 The block is **generated in place**, not written by hand:
 
-- **`tools/migrations.R`** is the single source of truth. Each entry declares the
-  function's `old` and `new` signatures as literal R functions; renames and
-  defaults are read straight off their formals (a bare-symbol default in `old`,
-  e.g. `c = c_renamed`, means a rename). The schema is documented at the top of
-  that file.
-- **`tools/generate-migrations.R`** reads the registry and rewrites the code
-  between the `# BEGIN GENERATED ARG_HANDLE: <fn>` / `# END GENERATED ARG_HANDLE`
-  markers inside each function. Output is idempotent (running twice produces no
-  diff) and laid out exactly as `air` formats it, so the host files stay clean.
+- **`tools/migrations/<topic>.R`** are the source of truth: one registry file
+  per topic, mirroring the R source files, so parallel migration efforts do
+  not conflict on a single file. Each entry declares the function's `old` and
+  `new` signatures as literal R functions; renames and defaults are read
+  straight off their formals (a bare-symbol default in `old`, e.g.
+  `c = c_renamed`, means a rename). A function may be declared in only one
+  registry file; the generator stops with an error on duplicates. The schema
+  is documented in [`tools/migrations/README.md`](migrations/README.md).
+- **`tools/generate-migrations.R`** reads all registry files and rewrites the
+  code between the `# BEGIN GENERATED ARG_HANDLE: <fn>` / `# END GENERATED
+  ARG_HANDLE` markers inside each function. Output is idempotent (running
+  twice produces no diff) and laid out exactly as `air` formats it, so the
+  host files stay clean.
 
 Regenerate after editing the registry:
 

tools/generate-migrations.R

@@ -2,8 +2,10 @@
 #
 # Generator for in-place argument-migration code.
 #
-# Reads the declarative registry in tools/migrations.R and splices recovery code
-# directly into each migrated function body, between the markers
+# Reads the declarative registry entries from the per-topic files in
+# tools/migrations/ (a legacy single-file tools/migrations.R is still honoured
+# if present, easing older branches over the transition) and splices recovery
+# code directly into each migrated function body, between the markers
 #
 #   # BEGIN GENERATED ARG_HANDLE: <fn>
 #   # END GENERATED ARG_HANDLE
@@ -31,38 +33,101 @@ migration_paths <- function() {
   # the package root.
   root <- getwd()
   list(
-    registry = file.path(root, "tools", "migrations.R"),
+    registry = migration_registry_files(root),
     out_dir = file.path(root, "R")
   )
 }
 
+# All registry sources: the legacy single file plus every file in
+# tools/migrations/, one per topic. Sorted for a deterministic load order.
+migration_registry_files <- function(root = getwd()) {
+  legacy <- file.path(root, "tools", "migrations.R")
+  topical <- sort(list.files(
+    file.path(root, "tools", "migrations"),
+    pattern = "\\.[rR]$",
+    full.names = TRUE
+  ))
+  c(if (file.exists(legacy)) legacy, topical)
+}
+
 # ---- registry loading + normalisation --------------------------------------
 
 load_migrations <- function(registry_path) {
+  loaded <- lapply(registry_path, load_migration_file)
+  migrations <- do.call(c, c(loaded, list(list())))
+  fns <- names(migrations)
+  dup <- unique(fns[duplicated(fns)])
+  if (length(dup)) {
+    stop(
+      "Duplicate migration entr",
+      if (length(dup) == 1L) "y" else "ies",
+      " across registry files: ",
+      paste(dup, collapse = ", "),
+      call. = FALSE
+    )
+  }
+  Map(normalise_migration, fns, migrations)
+}
+
+load_migration_file <- function(path) {
   env <- new.env(parent = baseenv())
-  sys.source(registry_path, envir = env, keep.source = FALSE)
+  sys.source(path, envir = env, keep.source = FALSE)
   if (!exists("migrations", envir = env, inherits = FALSE)) {
     stop(
       "`",
-      registry_path,
+      path,
       "` must define a `migrations` list.",
       call. = FALSE
     )
   }
   migrations <- get("migrations", envir = env)
+  if (length(migrations) == 0L) {
+    return(list())
+  }
   fns <- names(migrations)
   if (is.null(fns) || any(!nzchar(fns))) {
-    stop("`migrations` must be a list named by function.", call. = FALSE)
+    stop(
+      "`",
+      path,
+      "`: `migrations` must be a list named by function.",
+      call. = FALSE
+    )
   }
-  Map(normalise_migration, fns, migrations)
+  migrations
 }
 
 # Deparse a formal's default, force-safely. A formal with no default holds R's
 # "missing argument" sentinel, which errors the moment it is forced (e.g. by
 # `is.symbol()`); `deparse()` tolerates it and yields "". So we deparse first to
 # detect missingness, and only inspect the value once we know it is present.
 default_expr <- function(fmls, nm) {
-  paste(deparse(fmls[[nm]], width.cutoff = 500L), collapse = " ")
+  space_binary_slash(
+    paste(deparse(fmls[[nm]], width.cutoff = 500L), collapse = " ")
+  )
+}
+
+# air formats binary `/` with surrounding spaces, but deparse() emits `x/y`,
+# which would make the spliced blocks fail the idempotency check against the
+# air-formatted sources. Reinsert the spaces token-aware, so slashes inside
+# string literals (e.g. URLs) stay untouched.
+space_binary_slash <- function(text) {
+  if (!grepl("/", text, fixed = TRUE)) {
+    return(text)
+  }
+  pd <- tryCatch(
+    utils::getParseData(parse(text = text, keep.source = TRUE)),
+    error = function(e) NULL
+  )
+  if (is.null(pd)) {
+    return(text)
+  }
+  cols <- pd$col1[pd$terminal & pd$token == "'/'"]
+  if (length(cols) == 0L) {
+    return(text)
+  }
+  chars <- strsplit(text, "", fixed = TRUE)[[1]]
+  chars[cols] <- " / "
+  gsub("[ ]+/[ ]+", " / ", paste(chars, collapse = ""))
 }
 
 # Turn one registry entry (with `old`/`new` as function objects) into the flat
@@ -247,7 +312,8 @@ render_call_arg <- function(name, ctor, items, empty, trailing = ",") {
   n <- length(items)
   c(
     paste0(arg_indent, name, " = ", ctor, "("),
-    paste0(item_indent, items[-n], ","),
+    # paste0() would turn character(0) into "," for a single item
+    if (n > 1L) paste0(item_indent, items[-n], ","),
     paste0(item_indent, items[[n]]),
     paste0(arg_indent, ")", trailing)
   )