copy_model_from fails if parent model ID has period

[kyleam]

[ issue noticed while reviewing gh-614 ]

Periods are permitted as part of the model ID, and this is documented and tested for copy_model_from (and probably elsewhere):

bbr/R/copy-model-from.R

Lines 57 to 58 in c6a34e4

	#' # create model file at /path/to/100.1.ctl and YAML at /path/to/100.1.yaml
	#' copy_model_from(mod1, 100.1, "a period is okay")

bbr/tests/testthat/test-copy-model-from.R

Lines 155 to 173 in c6a34e4

	test_that("copy_model_from() supports `.new_model` containing a period [BBR-CMF-006]", {
	mod_content <- readLines(ctl_ext(MOD1_PATH)) %>% paste(collapse = "\n")
	temp_mod_path <- create_temp_model(mod_content = mod_content)
	temp_mod <- read_model(temp_mod_path)
	temp_dir <- normalizePath(tempdir())
	new_mod_path <- "foo.bar"
	new_ctl <- paste0(file.path(temp_dir, new_mod_path), ".ctl")
	new_yaml <- paste0(file.path(temp_dir, new_mod_path), ".yaml")
	expect_false(fs::file_exists(new_ctl))
	expect_false(fs::file_exists(new_yaml))
	on.exit(fs::file_delete(c(new_ctl, new_yaml)))
	new_mod <- copy_model_from(temp_mod, new_mod_path)
	expect_true(inherits(new_mod, NM_MOD_CLASS))
	expect_true(fs::file_exists(new_ctl))
	expect_true(fs::file_exists(new_yaml))
	})

However, if the parent model has a period, safe_based_on fails.

diff --git a/tests/testthat/test-copy-model-from.R b/tests/testthat/test-copy-model-from.R
index 3046f6c7..f867ec69 100644
--- a/tests/testthat/test-copy-model-from.R
+++ b/tests/testthat/test-copy-model-from.R
@@ -172,6 +172,15 @@ test_that("copy_model_from() supports `.new_model` containing a period [BBR-CMF-
   expect_true(fs::file_exists(new_yaml))
 })

+
+test_that("copy_model_from() supports parent model with period", {
+  withr::with_tempdir({
+    mod <- copy_model_from(MOD1, file.path(getwd(), "1.1"))
+    mod2 <- copy_model_from(mod, 2)
+    expect_identical(mod2[[YAML_BASED_ON]], "1.1")
+  })
+})
+
 test_that("copy_model_from(.new_model=NULL) increments to next integer by default [BBR-CMF-007]", {
   withr::defer(cleanup())
   new_id <- "002"

Error ('test-copy-model-from.R:179:5'): copy_model_from() supports parent model with period
Error: Parsed 1 models as `based_on` but cannot find .yaml files for 1 of them:  /tmp/RtmpMb714D/file6fac7d28f938/1.1
Backtrace:
     ▆
  1. ├─withr::with_tempdir(...) at test-copy-model-from.R:177:2
  2. │ └─withr::with_dir(tmp, code) at withr/R/tempfile.R:76:2
  3. │   └─base::force(code)
  4. ├─bbr::copy_model_from(mod, 2) at test-copy-model-from.R:179:4
  5. └─bbr:::copy_model_from.bbi_nonmem_model(mod, 2) at bbr/R/copy-model-from.R:73:2
  6.   └─bbr:::copy_model_from_impl(...) at bbr/R/copy-model-from.R:100:2
  7.     └─bbr::new_model(...) at bbr/R/copy-model-from.R:182:2
  8.       └─bbr::replace_all_based_on(.mod, .based_on) at bbr/R/new-model.R:74:2
  9.         ├─bbr::modify_model_field(...) at bbr/R/modify-model-field.R:298:2
 10.         │ └─base::unlist(.value) at bbr/R/modify-model-field.R:45:4
 11.         └─bbr:::safe_based_on(get_model_working_directory(.mod), .based_on)
 12.           └─bbr:::strict_mode_error(...) at bbr/R/modify-model-field.R:493:4

[kyleam]

safe_based_on processes the paths with yaml_ext, removing the ".1" and then goes looking for 1.yaml instead of 1.1:

bbr/R/modify-model-field.R

Lines 485 to 488 in c6a34e4

	.paths_bool <-
	.paths %>%
	purrr::map(yaml_ext) %>%
	purrr::map_lgl(fs::file_exists)

Fixing this probably involves a wider audit of how we handle this throughout the code base, but some notes:

• safe_based_on says that the paths can be with or without extension, so that's in some sense working as designed.

• safe_based_on is internal, but it directly processes user-supplied values in the add_based_on, replace_all_based_on, and remove_based_on, so it's connected to user-visible behavior.

• One fix would be to not permit an extension for the values given to the modify_based_on functions. The modify_based_on docs explicitly say that the based on field doesn't have an extension, but it doesn't specify that the input value can't. Presumably the reason safe_based_on exist is so that it can.

• Another option would to be more specific about what extensions are stripped (maybe ".mod", ".ctl", ".yaml"?).

---

Also note that bbr.bayes gq helpers follow the based-on handling fairly closely and likely have the same issues.