Show deprecation warning for Data Packages created with previous versions of frictionless

NEWS.md

@@ -2,29 +2,27 @@
 
 ## For users
 
-* `read_resource()` now supports reading from remote zip files, thanks to support in {vroom} (1.3.0) (#291).
+* `read_resource()` now supports reading from remote zip files, thanks to support in `{vroom}` (1.3.0) (#291).
 * `resources()` is soft-deprecated, please use `resource_names()` instead (#282).
 * `get_schema()` is soft-deprecated, please use `schema()` instead (#282).
 
 ## Changes for developers
 
-* Internal frictionless properties are now _attributes_, to better separate them from public Data Package properties (#289). If you use these internal properties, then update:
+* Internal frictionless properties `package$directory` and `resource$read_from` are now _attributes_ `attr(package, "directory")` and `attr(resource, "data_location")`. This separates them better from public Data Package and Resource _properties_ (#289). Saved Data Package objects created with previous versions of frictionless will show a deprecation warning (#293) and can be updated with `create_package()`. If you use these internal properties in your R package, then change them:
 
   ```R
+  # Before
   package$directory
-  r <- frictionless:::get_resource(package, "resource_name") # Internal function
-  r$read_from
-  ```
-
-  to:
+  resource <- frictionless:::get_resource(package, "resource_name") # Internal function
+  resource$read_from
 
-  ```R
+  # After
   attr(package, "directory")
-  r <- frictionless:::resource(package, "resource_name") # Renamed!
-  attr(r, "data_location") # Renamed!
+  resource <- frictionless:::resource(package, "resource_name") # Function renamed
+  attr(resource, "data_location") # Attribute renamed
   ```
 
-* frictionless now relies on R >= 4.1.0 (because of an indirect {vroom} dependency) (#291) and uses base pipes (`|>` rather than `%>%`) (#292).
+* frictionless now relies on R >= 4.1.0 (because of an indirect `{vroom}` dependency) (#291) and uses base pipes (`|>` rather than `%>%`) (#292).
 
 # frictionless 1.2.1
 

R/check_package.R

@@ -53,8 +53,17 @@ check_package <- function(package) {
     )
   }
 
-  # Check package has directory attribute (character)
-  if (!is.character(attr(package, "directory"))) {
+  # Check package has directory attribute (or deprecated package$directory)
+  if (is.character(package$directory)) {
+    lifecycle::deprecate_warn(
+      when = "1.3.0",
+      what = I("`package$directory`"),
+      details = cli::format_inline(
+        "This Data Package object was created with an older version of
+           frictionless. Update it with {.fun create_package}."
+      )
+    )
+  } else if (!is.character(attr(package, "directory"))) {
     cli::cli_abort(
       c(
         general_message,

R/create_package.R

@@ -39,8 +39,12 @@ create_package <- function(descriptor = NULL) {
   # Add resources property (also creates descriptor if NULL)
   descriptor$resources <- descriptor$resources %||% list()
 
-  # Add directory attribute
+  # Add directory attribute (and remove deprecated package$directory)
   attr(descriptor, "directory") <- attr(descriptor, "directory") %||% "."
+  if (is.character(descriptor$directory)) {
+    attr(descriptor, "directory") <- descriptor$directory
+    descriptor$directory <- NULL
+  }
 
   # Add datapackage class
   if (!"datapackage" %in% class(descriptor)) {

R/deprecated.R

@@ -11,9 +11,9 @@
 #' @name deprecated
 get_schema <- function(package, resource_name) {
   lifecycle::deprecate_soft(
-    "1.3.0",
-    "get_schema()",
-    "schema()"
+    when = "1.3.0",
+    what = "get_schema()",
+    with = "schema()"
   )
   schema(package, resource_name)
 }
@@ -27,9 +27,9 @@ get_schema <- function(package, resource_name) {
 #' @name deprecated
 resources <- function(package) {
   lifecycle::deprecate_soft(
-    "1.3.0",
-    "resources()",
-    "resource_names()"
+    when = "1.3.0",
+    what = "resources()",
+    with = "resource_names()"
   )
   resource_names(package)
 }

tests/testthat/test-check_package.R

@@ -37,53 +37,68 @@ test_that("check_package() returns error if package is not a list", {
 })
 
 test_that("check_package() returns error on missing or incorrect resources", {
+  p_invalid <- create_package()
+  p_invalid$resources <- NULL
   expect_error(
-    check_package(list()),
+    check_package(p_invalid),
     class = "frictionless_error_package_invalid"
   )
+  p_invalid$resources <- "not_a_list"
   expect_error(
-    check_package(list(resources = "not_a_list")),
+    check_package(p_invalid),
     regexp = "`package` is missing a resources property or it is not a list.",
     fixed = TRUE
   )
   expect_error(
-    check_package(list(resources = "not_a_list")),
+    check_package(p_invalid),
     class = "frictionless_error_package_invalid"
   )
 })
 
 test_that("check_package() returns error on missing or incorrect directory", {
+  p_invalid <- create_package()
+  attr(p_invalid, "directory") <- NULL
   expect_error(
-    check_package(list(resources = list())),
+    check_package(p_invalid),
     class = "frictionless_error_package_invalid"
   )
   expect_error(
-    check_package(list(resources = list())),
+    check_package(p_invalid),
     regexp = paste(
       "`package` is missing a directory attribute or it is not a character."
     ),
     fixed = TRUE
   )
   expect_error(
-    check_package(list(resources = list(), directory = 5)),
+    check_package(p_invalid),
     class = "frictionless_error_package_invalid"
   )
 })
 
+test_that("check_package() returns deprecation warning for package$directory", {
+  p_directory <- example_package()
+  p_directory$directory <- attr(p_directory, "directory")
+  attr(p_directory, "directory") <- NULL
+
+  lifecycle::expect_deprecated(check_package(p_directory))
+  expect_no_error(suppressWarnings(check_package(p_directory)))
+})
+
 test_that("check_package() returns error if resources have no name", {
-  p <- example_package()
-  p$resources[[2]]$name <- NULL
+  p_invalid <- example_package()
+  p_invalid$resources[[2]]$name <- NULL
   expect_error(
-    check_package(p),
+    check_package(p_invalid),
     class = "frictionless_error_resources_without_name"
   )
   expect_error(
-    check_package(p),
+    check_package(p_invalid),
     regexp = "All resources in `package` must have a name property.",
     fixed = TRUE
   )
 
   # Expect no error on empty resources
+  p <- example_package()
   p$resources <- list()
   expect_no_error(check_package(p))
 })

tests/testthat/test-create_package.R

@@ -43,6 +43,16 @@ test_that("create_package() sets directory or leaves as is", {
   expect_identical(attr(existing, "directory"), "not_default")
 })
 
+test_that("create_package() updates deprecated package$directory", {
+  p_directory <- example_package()
+  p_directory$directory <- "not_default"
+  attr(p_directory, "directory") <- NULL
+  p_updated <- create_package(p_directory)
+
+  expect_null(p_updated$directory)
+  expect_identical(attr(p_updated, "directory"), "not_default")
+})
+
 test_that("create_package() adds class 'datapackage'", {
   new <- create_package()
   expect_s3_class(new, "datapackage")