Fix compare digests (#95)

* Author changes in description file * Dev version bump * update compare for same number of dataobjects * Update CONTRIBUTING.md with Git workflow guidlines and CI reminder. (#87) Co-authored-by: Jason Taylor jmtaylor@fredhutch.org * Vignette edits (#92) * text edits to vignettes * match casing for vignette names * rm non-rmd files in vignettes, add .gitignore to vignettes * add style pointers for vignettes to contributing * alter valid name expression to allow for different names in each set * add tests for .compare_digests * Usethis patch (#74) * Patch to address #73 * bump version, update travis, test locally, rerun codemeta. #73 * imports for withr for #73 * update news. #73 * refactor logic of .compare_digests, fix typo package_build help * add test for .compare_digests refactor * Add maintainer email and role. * improve test coverage * new_digest zero length check in .check_digests not needed, removed * specify serialization for backward compatibility (#94) * specify serialization for backward compatibility * removed whitespace edits Co-authored-by: Jason Taylor <jmtaylor@fredhutch.org> * add condition for checking for removed data in digest * fix list subsetting, update tests * add back deleted dataversion.R Co-authored-by: William Fulp <wfulp@fredhutch.org> Co-authored-by: jmtaylor-fhcrc <42221209+jmtaylor-fhcrc@users.noreply.github.com> Co-authored-by: Jason Taylor <jmtaylor@fredhutch.org> Co-authored-by: Greg Finak <greg.finak@gmail.com> Co-authored-by: Jimmy Fulp <WilliamJFulp@gmail.com>
ropensci · Apr 5, 2022 · 1bfcf63 · 1bfcf63
1 parent 8c443ca
commit 1bfcf63
Show file tree

Hide file tree

Showing 22 changed files with 338 additions and 2,573 deletions.
diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md
@@ -6,7 +6,7 @@ Small typos or grammatical errors in documentation may be edited directly using
 the GitHub web interface, so long as the changes are made in the _source_ file.
 
 *  YES: you edit a roxygen comment in a `.R` file below `R/`.
-*  NO: you edit an `.Rd` file below `man/`.
+*  NO: you should not edit an `.Rd` file below `man/`.
 
 ### Prerequisites
 
@@ -17,8 +17,11 @@ bug, create an associated issue and illustrate the bug with a minimal
 
 ### Pull request process
 
-*  We recommend that you create a Git branch for each pull request (PR).  
-*  Look at the Travis and AppVeyor build status before and after making changes.
+*  We are using the Git commit workflow found here: 
+https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow  
+*  We recommend that you create a Git branch for each pull request (PR) and keep
+issues addressed to one issue per branch.  
+*  Look at the git workflow checks before and after making changes.
 The `README` should contain badges for any continuous integration services used
 by the package.  
 *  We recommend the tidyverse [style guide](https://style.tidyverse.org).
@@ -41,6 +44,22 @@ project you agree to abide by its terms.
 ### See rOpenSci [contributing guide](https://ropensci.github.io/dev_guide/contributingguide.html)
 for further details.
 
+### Style pointers for vignettes
+
+* Headings are in sentence case and contain a period at the end of the heading when appropriate.
+* Conjunctions are excluded from text.
+* Multiline function calls use the following whitespace schema:
+
+``` R
+myFunction <- aFunctionCall(
+  input1 = "this",
+  input2 = "that"
+)
+```
+
+* Vignette file names are snake cased with capital first letters: Like_This.Rmd
+* Vignette `VignetteIndexEntry` are in sentence case: My new vignette
+
 ### Discussion forum
 
 Check out our [discussion forum](https://discuss.ropensci.org) if you think your issue requires a longer form discussion.

diff --git a/DESCRIPTION b/DESCRIPTION
@@ -7,15 +7,15 @@ Authors@R:
              email="greg.finak@gmail.com",
              comment="Original author and creator of DataPackageR"),
       person(given = "Paul Obrecht", role=c("ctb")),
-      person(given = "Ellis Hughes", role=c("ctb","cre"), email = "ehhughes@scharp.org"),
+      person(given = "Ellis Hughes", role=c("ctb"), email = "ellishughes@live.com"),
       person(given = "Jimmy Fulp", role=c("ctb"), email = "wfulp@scharp.org"),
-      person(given = "Marie Vendettuoli", role=c("ctb"), email = "mvendett@scharp.org"),
-      person(given = "Jason Taylor", role=c("ctb")),
+      person(given = "Marie Vendettuoli", role=c("ctb", "cre"), email = "sc.mvendett.viscdevelopers@scharp.org"),
+      person(given = "Jason Taylor", role=c("ctb"), email = "jmtaylor@fredhutch.org"),
       person("Kara", "Woo", role = "rev",
       comment = "Kara reviewed the package for ropensci, see <https://github.com/ropensci/onboarding/issues/230>"),
       person("William", "Landau", role = "rev",
       comment = "William reviewed the package for ropensci, see <https://github.com/ropensci/onboarding/issues/230>"))
-Version: 0.15.8
+Version: 0.15.8.9000
 Description: A framework to help construct R data packages in a 
   reproducible manner. Potentially time consuming processing of 
   raw data sets into analysis ready data sets is done in a reproducible 
@@ -45,7 +45,8 @@ Imports:
     futile.logger,
     rprojroot,
     usethis,
-    crayon
+    crayon,
+    withr
 VignetteBuilder: knitr
 RoxygenNote: 7.1.1
 Encoding: UTF-8

diff --git a/NAMESPACE b/NAMESPACE
@@ -75,6 +75,7 @@ importFrom(utils,modifyList)
 importFrom(utils,package.skeleton)
 importFrom(utils,packageDescription)
 importFrom(utils,packageVersion)
+importFrom(withr,with_options)
 importFrom(yaml,as.yaml)
 importFrom(yaml,read_yaml)
 importFrom(yaml,write_yaml)

diff --git a/NEWS.md b/NEWS.md
@@ -1,3 +1,6 @@
+# DataPackageR 0.15.8.9000
+* Fix tests for compatibility with usethis >1.5.2
+
 # DataPackageR 0.15.8
 * Fix to datapackager_object_read that was causing a test to break. `get` needs to have `inherits=FALSE`. 
 * Other fixes for `usethis` 1.6.0

diff --git a/R/build.R b/R/build.R
@@ -8,7 +8,7 @@
 #' @param vignettes \code{logical} specify whether to build vignettes. Default FALSE.
 #' @param log log level \code{INFO,WARN,DEBUG,FATAL}
 #' @param deps \code{logical} should we pass data objects into subsequent scripts? Default TRUE
-#' @param install \code{logical} automatically install and load the package after building. (default TRUE)
+#' @param install \code{logical} automatically install and load the package after building. Default FALSE
 #' @param ... additional arguments passed to \code{install.packages} when \code{install=TRUE}.
 #' @importFrom roxygen2 roxygenise roxygenize
 #' @importFrom devtools build_vignettes build parse_deps reload

diff --git a/R/dataversion.r → R/dataversion.R b/R/dataversion.r → R/dataversion.R
diff --git a/R/digests.R b/R/digests.R
@@ -30,42 +30,38 @@
 }
 
 .compare_digests <- function(old_digest, new_digest) {
-  valid <- ifelse(length(old_digest) != length(new_digest), FALSE, TRUE)
-  if (valid) {
-    for (i in names(new_digest)[-1L]) {
-      if (new_digest[[i]] != old_digest[[i]]) {
-        .multilog_warn(paste0(i, " has changed."))
-        valid <- FALSE
+  # Returns FALSE when any exisiting data has is changed, new data is added, or data is removed, else return TRUE.
+  # Use .mutlilog_warn when there is a change and multilog_debug when new data is added.
+
+  existed <- names(new_digest)[names(new_digest) %in% names(old_digest)]
+  added <- setdiff(names(new_digest), existed)
+  removed <- names(old_digest)[!names(old_digest) %in% names(new_digest)]
+  existed <- existed[existed != "DataVersion"]  
+
+  if(length(existed) > 0){
+    changed <- names(new_digest)[ unlist(new_digest[existed]) != unlist(old_digest[existed]) ] 
+    if(length(changed) > 0){
+      for(name in changed){
+        .multilog_warn(paste(name, "has changed."))
       }
-    }
-    if (!valid) {
       warning()
-    }
-  } else {
-    difference <- setdiff(names(new_digest), names(old_digest))
-    intersection <- intersect(names(new_digest), names(old_digest))
-    # No existing or new objects are changed
-    if (length(difference) == 0) {
-      valid <- TRUE
-    } else {
-      # some new elements exist
-      valid <- FALSE
-      for (i in difference) {
-        .multilog_debug(paste0(i, " added."))
-      }
-    }
-    for (i in intersection) {
-      if (new_digest[[i]] != old_digest[[i]]) {
-        .multilog_debug(paste0(i, " changed"))
-        # some new elements are not the same
-        valid <- FALSE
-      }
+      return(FALSE)
     }
   }
-  return(valid)
-}
 
+  for(name in removed){
+    .multilog_debug(paste(name, "was removed."))
+    return(FALSE)
+  }
+
+  for(name in added){
+    .multilog_debug(paste(name, "was added."))
+    return(FALSE)
+  }
 
+  return(TRUE)
+};
+
 .combine_digests <- function(new, old) {
   intersection <- intersect(names(new), names(old))
   difference <- setdiff(names(new), names(old))

diff --git a/R/processData.R b/R/processData.R
@@ -360,7 +360,8 @@ DataPackageR <- function(arg = NULL, deps = TRUE) {
           assign(o, get(o, dataenv), ENVS)
           # write the object to render_root
           o_instance <- get(o,dataenv)
-          saveRDS(o_instance, file = paste0(file.path(render_root,o),".rds"))
+          saveRDS(o_instance, file = paste0(file.path(render_root,o),".rds"),
+                  version = 2)
         }
       }
     }

diff --git a/R/yamlR.R b/R/yamlR.R
@@ -7,6 +7,7 @@
 #' @details Add, remove files and objects, enable or disable parsing of specific files,  list objects or files in a yaml config, or write a config back to a package.
 #' @importFrom yaml yaml.load_file as.yaml write_yaml
 #' @importFrom stats runif
+#' @importFrom withr with_options 
 #' @export
 #'
 #' @examples

diff --git a/codemeta.json b/codemeta.json
@@ -295,6 +295,18 @@
       },
       "sameAs": "https://CRAN.R-project.org/package=crayon"
     },
+    {
+      "@type": "SoftwareApplication",
+      "identifier": "withr",
+      "name": "withr",
+      "provider": {
+        "@id": "https://cran.r-project.org",
+        "@type": "Organization",
+        "name": "Comprehensive R Archive Network (CRAN)",
+        "url": "https://cran.r-project.org"
+      },
+      "sameAs": "https://CRAN.R-project.org/package=withr"
+    },
     {
       "@type": "SoftwareApplication",
       "identifier": "https://sysreqs.r-hub.io/get/pandoc"

diff --git a/tests/testthat/test-data-name-change.R b/tests/testthat/test-data-name-change.R
@@ -0,0 +1,61 @@
+test_that("data object can be renamed", {
+
+  addData <- function(dataset, pname){
+    fil <- sprintf("data(%s, envir=environment())", dataset)
+    writeLines(fil, file.path(tempdir(), sprintf("%s/data-raw/%s.R", pname, dataset)))
+
+    yml <- yml_add_files(file.path(tempdir(), pname), c(sprintf("%s.R", dataset)))
+    yml <- yml_add_objects(yml, dataset)
+    yml_write(yml)
+
+    package_build(file.path(tempdir(), pname))
+  }
+
+  changeName <- function(old_dataset_name, new_dataset_name, pname){
+    process_path <- file.path(tempdir(), sprintf("%s/data-raw/%s.R", pname, old_dataset_name))
+    fil <- c(readLines(process_path), sprintf("%s <- %s", new_dataset_name, old_dataset_name))
+    writeLines(fil, process_path)
+
+    yml <- yml_remove_objects(file.path(tempdir(), pname), old_dataset_name)
+    yml <- yml_add_objects(yml, new_dataset_name)
+    yml_write(yml)
+
+    package_build(file.path(tempdir(), pname))
+  }
+
+  removeName <- function(dataset_name, script, pname){
+    process_path <- file.path(tempdir(), sprintf("%s/data-raw/%s", pname, script))
+    fil <- gsub(paste0("^", dataset_name, ".+$"), "", readLines(process_path))
+    writeLines(fil, process_path)
+
+    yml <- yml_remove_objects(file.path(tempdir(), pname), dataset_name)
+    yml_write(yml)
+
+    package_build(file.path(tempdir(), pname))
+  }
+
+  ## test change when one object is present 
+  pname <- "nameChangeTest1"
+  datapackage_skeleton(pname, tempdir(), force = TRUE)
+  addData("mtcars", pname)
+  expect_error(changeName("mtcars", "mtcars2", pname), NA)
+  expect_error(removeName("mtcars2", "mtcars.R", pname), "exiting")
+
+  ## test change when two objects are present 
+  pname <- "nameChangeTest2"
+  datapackage_skeleton(pname, tempdir(), force = TRUE)
+  addData("mtcars", pname)
+  addData("iris", pname)
+  expect_error(changeName("mtcars", "mtcars2", pname), NA)
+  expect_error(removeName("mtcars2", "mtcars.R", pname), NA)
+
+  ## test change when more than 2 objects are present 
+  pname <- "nameChangeTest3"
+  datapackage_skeleton(pname, tempdir(), force = TRUE)
+  addData("mtcars", pname)
+  addData("iris", pname)
+  addData("ToothGrowth", pname)
+  expect_error(changeName("mtcars", "mtcars2", pname), NA)
+  expect_error(removeName("mtcars2", "mtcars.R", pname), NA)
+
+})
diff --git a/tests/testthat/test-edge-cases.R b/tests/testthat/test-edge-cases.R
@@ -171,7 +171,8 @@ test_that("package built in different edge cases", {
   package.skeleton("foo", path = tempdir())
   DataPackageR:::.multilog_setup(normalizePath(file.path(tempdir(),"test.log"), winslash = "/"))
   DataPackageR:::.multilog_thresold(INFO, TRACE)
-
+
+  # data in digest changes while names do not
   suppressWarnings(expect_false({
     DataPackageR:::.compare_digests(
       list(
@@ -185,6 +186,64 @@ test_that("package built in different edge cases", {
     )
   }))
 
+  # names in digest changes while data do not
+  suppressWarnings(expect_false({
+    DataPackageR:::.compare_digests(
+      list(
+        DataVersion = "1.1.1",
+        a = paste0(letters[1:10], collapse = "")
+      ),
+      list(
+        DataVersion = "1.1.2",
+        b = paste0(letters[1:10], collapse = "")
+      )
+    )
+  }))
+
+  # names in digest nor data changes
+  suppressWarnings(expect_true({
+    DataPackageR:::.compare_digests(
+      list(
+        DataVersion = "1.1.1",
+        a = paste0(letters[1:10], collapse = "")
+      ),
+      list(
+        DataVersion = "1.1.1",
+        a = paste0(letters[1:10], collapse = "")
+      )
+    )
+  }))
+
+  # names in old digest have one more than new
+  suppressWarnings(expect_false({
+    DataPackageR:::.compare_digests(
+      list(
+        DataVersion = "1.1.1",
+        a = paste0(letters[1:10], collapse = ""),
+        b = paste0(LETTERS[1:10], collapse = "")
+      ),
+      list(
+        DataVersion = "1.1.2",
+        a = paste0(letters[1:10], collapse = "")
+      )
+    )
+  }))
+
+  # names in new digest have one more than old
+  suppressWarnings(expect_false({
+    DataPackageR:::.compare_digests(
+      list(
+        DataVersion = "1.1.1",
+        a = paste0(letters[1:10], collapse = "")
+      ),
+      list(
+        DataVersion = "1.1.2",
+        a = paste0(letters[1:10], collapse = ""),
+        b = paste0(LETTERS[1:10], collapse = "")
+      )
+    )
+  }))
+
   unlink(file.path(tempdir(), "foo"),
     force = TRUE,
     recursive = TRUE

diff --git a/tests/testthat/test-ignore.R b/tests/testthat/test-ignore.R
@@ -13,6 +13,9 @@ test_that("use_ignore works", {
       r_object_names = c("cars_over_20")
     )
   )
-  expect_invisible(use_ignore(file = "mydata.csv",path = "inst/extdata"))#,"Adding 'mydata.csv' to 'inst/extdata/\\.gitignore'|Adding '\\^inst/extdata/mydata\\\\.csv\\$' to '\\.Rbuildignore'")
+  #expect_output(use_ignore(file = "mydata.csv",path = "inst/extdata"),"Adding 'mydata.csv' to 'inst/extdata/\\.gitignore'|Adding '\\^inst/extdata/mydata\\\\.csv\\$' to '\\.Rbuildignore'")
+  withr::with_options(c(crayon.enabled = FALSE), {
+	expect_message(use_ignore(file = "mydata.csv", path = "inst/extdata"), "Adding 'mydata.csv' to 'inst/extdata/\\.gitignore'|Adding '\\^inst/extdata/mydata\\.csv\\$' to '\\.Rbuildignore'")
+  })
   expect_message(use_ignore(),"No file name provided to ignore.")
 })
diff --git a/vignettes/.gitignore b/vignettes/.gitignore
@@ -0,0 +1,3 @@
+*.R
+*.md
+*.html
-Original file line number
+Diff line change
@@ -0,0 +1,3 @@
+    *.R
+    *.md
+    *.html