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):

https://github.com/metrumresearchgroup/bbr/blob/c6a34e43bedb335bb5bd42edc6c4ac760c1a8cea/R/copy-model-from.R#L57-L58

https://github.com/metrumresearchgroup/bbr/blob/c6a34e43bedb335bb5bd42edc6c4ac760c1a8cea/tests/testthat/test-copy-model-from.R#L155-L173

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:

https://github.com/metrumresearchgroup/bbr/blob/c6a34e43bedb335bb5bd42edc6c4ac760c1a8cea/R/modify-model-field.R#L485-L488

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.