fix: preserve pinned versions in DESCRIPTION and accept several dirs in dir.r (#139, #140)

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: attachment
 Title: Deal with Dependencies
-Version: 1.0.1
+Version: 1.1.0
 Authors@R: c(
     person("Vincent", "Guyader", , "vincent@thinkr.fr", role = c("cre", "aut"),
            comment = c(ORCID = "0000-0003-0671-9270")),

NEWS.md

@@ -1,7 +1,21 @@
-# attachment 1.0.1
+# attachment 1.1.0
+
+## New features
+
+- `att_amend_desc()` now accepts several directories in `dir.r`, for example
+  `att_amend_desc(dir.r = c("R", "inst"))`. This lets you declare dependencies
+  used only outside the standard package directories, such as a deployment
+  entry point under `inst/`, which the default scan does not reach. Passing a
+  vector previously raised `the condition has length > 1` (#139).
 
 ## Bug fixes
 
+- `att_amend_desc()` no longer drops a version constraint set by hand in
+  DESCRIPTION when the same package is listed under two types (a pinned
+  `Imports` and a bare `Suggests`, for instance). Previously the version that
+  survived depended on the order of the rows in DESCRIPTION; the hand-set
+  constraint is now kept, and when both an `Imports` and a `Suggests` row carry
+  a version the `Imports` one wins (#140).
 - `att_from_examples()` (and therefore `att_amend_desc()`) no longer chokes
   on inline R inside roxygen2 markdown tags such as
   `@param x \`r helper("x")\`` when `helper()` is a package-local function.

R/att_to_description.R

@@ -7,7 +7,12 @@
 #'
 #' @param path path to the root of the package directory. Default to current directory.
 #' @param path.n path to namespace file.
-#' @param dir.r path to directory with R scripts.
+#' @param dir.r Character vector of one or more directories holding R scripts to
+#'   parse for dependencies. Defaults to `"R"`. Pass several paths, for example
+#'   `c("R", "inst")`, to also scan sources that live outside the standard
+#'   package directories (a deployment entry point under `inst/`, say). Packages
+#'   called with `library()` or `pkg::fun()` in these scripts are added to
+#'   Imports, so a version already pinned for them in DESCRIPTION is preserved.
 #' @param dir.v path to vignettes directory. Set to empty (dir.v = "") to ignore.
 #' @param dir.t path to tests directory. Set to empty (dir.t = "") to ignore.
 #' @param extra.suggests vector of other packages that should be added in Suggests (pkgdown, covr for instance)
@@ -190,7 +195,7 @@ att_amend_desc <- function(path = ".",
   if (path.n != "") {
     imports <- unique(c(imports, att_from_namespace(path.n, document = document)))
   }
-  if (dir.r != "") {
+  if (!identical(dir.r, "")) {
     # Look for R scripts
     imports <- unique(c(imports, att_from_rscripts(dir.r)))
     # Look for Rmd, in case in a bookdown
@@ -201,7 +206,7 @@ att_amend_desc <- function(path = ".",
   suggests <- NULL
 
   # Get suggests in examples and remove if already in imports
-  if (dir.r != "") {
+  if (!identical(dir.r, "")) {
     ex <- att_from_examples(dir.r = dir.r)
     suggests <- c(suggests, ex[!ex %in% imports])
   }
@@ -364,10 +369,21 @@ att_to_desc_from_is <- function(path.d = "DESCRIPTION", imports = NULL,
   all_packages <- c(imports, suggests)
   if (is.null(all_packages)) {all_packages <- character()}
 
+  # Collapse the original versions to one row per package before the join.
+  # A package listed under two types (a pinned Imports and a bare Suggests, say)
+  # otherwise multiplies rows in the merge, and the later de-duplication would
+  # keep whichever row happened to come first. Order so that an explicit
+  # constraint beats "*", and Imports beats Suggests on a tie, so a version set
+  # by hand in DESCRIPTION is never silently dropped.
+  orig_versions <- deps_orig[
+    order(deps_orig$version == "*", deps_orig$type),
+    c("package", "version")]
+  orig_versions <- orig_versions[!duplicated(orig_versions$package), ]
+
   deps_new <- data.frame(
     type = c(rep("Imports", length(imports)), rep("Suggests", length(suggests))),
     package = all_packages, stringsAsFactors = FALSE) %>%
-    merge(deps_orig[,c("package", "version")],
+    merge(orig_versions,
           by = "package", sort = TRUE, all.x = TRUE, all.y = FALSE) %>%
     .[,c("type", "package", "version")] %>%
     .[order(.$type, .$package), , drop = FALSE] %>%

tests/testthat/test-att_amend_desc_version_pins.R

@@ -0,0 +1,116 @@
+# Version constraints already set in DESCRIPTION must survive att_amend_desc(),
+# and dir.r must accept several directories (issues #139 and #140).
+
+copy_dummy <- function() {
+  tmpdir <- tempfile("dummypins")
+  dir.create(tmpdir)
+  file.copy(system.file("dummypackage", package = "attachment"), tmpdir, recursive = TRUE)
+  file.path(tmpdir, "dummypackage")
+}
+
+test_that("att_amend_desc keeps a hand-set version when the package also sits under another type (#140)", {
+  dummypackage <- copy_dummy()
+  on.exit(unlink(dirname(dummypackage), recursive = TRUE), add = TRUE)
+
+  # `glue` is detected as a Suggests dependency in dummypackage. Pin it in Imports by hand so
+  # it appears under two types in DESCRIPTION before the amend.
+  d <- desc::desc(file = file.path(dummypackage, "DESCRIPTION"))
+  d$set_dep("glue", type = "Imports", version = ">= 1.2.0")
+  d$write()
+
+  att_amend_desc(
+    path = dummypackage,
+    document = FALSE,
+    check_if_suggests_is_installed = FALSE,
+    use.config = FALSE
+  )
+
+  deps <- desc::desc_get_deps(file.path(dummypackage, "DESCRIPTION"))
+  glue_row <- deps[deps$package == "glue", ]
+
+  expect_equal(nrow(glue_row), 1L)
+  # glue is used from a Suggests location, so the scan moves it there; the
+  # hand-set version must follow it rather than being reset to "*".
+  expect_equal(glue_row$type, "Suggests")
+  expect_equal(glue_row$version, ">= 1.2.0")
+})
+
+test_that("att_amend_desc keeps the Imports-side version when both types carry different pins (#140)", {
+  dummypackage <- copy_dummy()
+  on.exit(unlink(dirname(dummypackage), recursive = TRUE), add = TRUE)
+
+  # A different explicit constraint under each type. Collapsing to one row forces
+  # a choice: the Imports-side constraint wins, whatever the row order.
+  d <- desc::desc(file = file.path(dummypackage, "DESCRIPTION"))
+  d$set_dep("glue", type = "Imports", version = ">= 1.2.0")
+  d$set_dep("glue", type = "Suggests", version = ">= 9.9.9")
+  d$write()
+
+  att_amend_desc(
+    path = dummypackage,
+    document = FALSE,
+    check_if_suggests_is_installed = FALSE,
+    use.config = FALSE
+  )
+
+  deps <- desc::desc_get_deps(file.path(dummypackage, "DESCRIPTION"))
+  glue_row <- deps[deps$package == "glue", ]
+
+  expect_equal(nrow(glue_row), 1L)
+  expect_equal(glue_row$version, ">= 1.2.0")
+})
+
+test_that("att_amend_desc resolves an Imports/Suggests conflict to Imports, keeping the version (#140)", {
+  dummypackage <- copy_dummy()
+  on.exit(unlink(dirname(dummypackage), recursive = TRUE), add = TRUE)
+
+  # `fakepkg` is really used in R/ code, so the scan classifies it as an Imports dependency.
+  r_file <- file.path(dummypackage, "R", "fun_pin.R")
+  writeLines(
+    text = "#' @export\nuse_fake <- function() {\n  fakepkg::run()\n}",
+    con = r_file
+  )
+  # DESCRIPTION carries it under both types: pinned Imports and bare Suggests.
+  d <- desc::desc(file = file.path(dummypackage, "DESCRIPTION"))
+  d$set_dep("fakepkg", type = "Imports", version = ">= 2.0.0")
+  d$set_dep("fakepkg", type = "Suggests", version = "*")
+  d$write()
+
+  att_amend_desc(
+    path = dummypackage,
+    document = FALSE,
+    must.exist = FALSE,
+    check_if_suggests_is_installed = FALSE,
+    use.config = FALSE
+  )
+
+  deps <- desc::desc_get_deps(file.path(dummypackage, "DESCRIPTION"))
+  fake_row <- deps[deps$package == "fakepkg", ]
+
+  expect_equal(nrow(fake_row), 1L)
+  expect_equal(fake_row$type, "Imports")
+  expect_equal(fake_row$version, ">= 2.0.0")
+})
+
+test_that("att_amend_desc accepts several directories in dir.r (#139)", {
+  dummypackage <- copy_dummy()
+  on.exit(unlink(dirname(dummypackage), recursive = TRUE), add = TRUE)
+
+  # A runtime dependency used only outside the default scan (here inst/).
+  dir.create(file.path(dummypackage, "inst"), showWarnings = FALSE)
+  writeLines("clipr::write_clip('x')", file.path(dummypackage, "inst", "main.R"))
+
+  expect_no_error(
+    att_amend_desc(
+      path = dummypackage,
+      dir.r = c("R", "inst"),
+      document = FALSE,
+      must.exist = FALSE,
+      check_if_suggests_is_installed = FALSE,
+      use.config = FALSE
+    )
+  )
+
+  deps <- desc::desc_get_deps(file.path(dummypackage, "DESCRIPTION"))
+  expect_true("clipr" %in% deps$package)
+})

vignettes/a-fill-pkg-description.Rmd

@@ -121,6 +121,16 @@ Then I can directly run `att_amend_desc()`.
 
 In the _Imports_ case, if for any reason I decide to delete this `my_knit()` function, then the {bookdown} dependency won't be needed anymore, and {attachment} will automatically remove it with the next `att_amend_desc()`
 
+### I have an R script in my "inst/" directory
+
+If the file under "inst/" is a plain R script rather than a notebook, for instance a deployment entry point "inst/main.R" that calls `pkg::fun()`, you can have {attachment} scan it by adding its directory to `dir.r`:
+
+```{r, eval=FALSE}
+att_amend_desc(dir.r = c("R", "inst"))
+```
+
+Packages detected this way are added to "Imports". A version already pinned by hand in DESCRIPTION for such a package is preserved.
+
 
 ## Example on a fake package