Merge pull request #169 from frictionlessdata/cli_resource

Use cli_abort(), drop assertthat for most resource related functions

R/add_resource.R

@@ -81,28 +81,33 @@ add_resource <- function(package, resource_name, data, schema = NULL,
   check_package(package)
 
   # Check resource name
-  assertthat::assert_that(
-    grepl(resource_name, pattern = "^[a-z0-9\\._-]+$"),
-    msg = glue::glue(
-      "`resource_name` `{resource_name}` must only contain lowercase",
-      "alphanumeric characters plus `.`, `-` and `_`.",
-      .sep = " "
+  if (!grepl(resource_name, pattern = "^[a-z0-9\\._-]+$")) {
+    cli::cli_abort(
+      c(
+        "{.arg resource_name} must only consist of lowercase alphanumeric
+         characters, {.val .}, {.val -} and {.val _}.",
+        "x" = "{.val {resource_name}} does not meet those criteria."
+      ),
+      class = "frictionless_error_resource_name_invalid"
     )
-  )
+  }
 
   # Check resource is absent
-  assertthat::assert_that(
-    !resource_name %in% resources(package),
-    msg = glue::glue(
-      "`package` already contains a resource named `{resource_name}`."
+  if (resource_name %in% resources(package)) {
+    cli::cli_abort(
+      "{.arg package} already contains a resource named {.val {resource_name}}.",
+      class = "frictionless_error_resource_already_exists"
+    )
+  }
+
+  # Check data (data frame or path), content of data frame is checked later
+  if (!is.data.frame(data) && !is.character(data)) {
+    cli::cli_abort(
+      "{.arg data} must either be a data frame or path(s) to CSV file(s).",
+      class = "frictionless_error_data_type_invalid"
     )
-  )
+  }
 
-  # Check data (df or path)
-  assertthat::assert_that(
-    is.data.frame(data) | is.character(data),
-    msg = "`data` must be a data frame or path(s) to CSV file(s)."
-  )
   if (is.data.frame(data)) {
     df <- data
   } else {
@@ -129,22 +134,27 @@ add_resource <- function(package, resource_name, data, schema = NULL,
   check_schema(schema, df)
 
   # Check ellipsis
-  assertthat::assert_that(
-    ...length() == length(...names()),
-    msg = "All arguments in `...` must be named."
-  )
+  if (...length() != length(...names())) {
+    cli::cli_abort(
+      "All arguments in {.arg ...} must be named.",
+      class = "frictionless_error_unnamed_argument"
+    )
+  }
   properties <- ...names()
   reserved_properties <- c(
     "name", "path", "profile", "format", "mediatype", "encoding", "dialect"
   ) # data and schema are also reserved, but are named arguments
   conflicting_properties <- properties[properties %in% reserved_properties]
-  assertthat::assert_that(
-    length(conflicting_properties) == 0,
-    msg = glue::glue(
-      "`{conflicting_properties[1]}` must be removed as an argument. ",
-      "It is automatically added as a resource property by the function."
+  if (length(conflicting_properties) != 0) {
+    cli::cli_abort(
+      c(
+        "{.arg {conflicting_properties}} must be removed as argument{?s}.",
+        "i" = "{.field {conflicting_properties}} {?is/are} automatically added
+               as resource propert{?y/ies}."
+      ),
+      class = "frictionless_error_resource_properties_reserved"
     )
-  )
+  }
 
   # Create resource, with properties in specific order
   if (is.data.frame(data)) {

R/check_schema.R

@@ -47,7 +47,7 @@ check_schema <- function(schema, data = NULL) {
   if (length(invalid_types) > 0) {
     cli::cli_abort(
       c(
-        "All fields in {.arg schema} must have a valid {.field type}.",
+        "All fields in {.arg schema} must have a valid {.field type} property.",
         "x" = "Type{?s} {.val {invalid_types}} {?is/are} invalid."
       ),
       class = "frictionless_error_fields_type_invalid"
@@ -63,8 +63,8 @@ check_schema <- function(schema, data = NULL) {
       cli::cli_abort(
         c(
           "Field names in {.arg schema} must match column names in {.arg data}.",
-          "i" = "Field name{?s}: {.val {field_names}}",
-          "i" = "Column name{?s}: {.val {col_names}}"
+          "i" = "Field name{?s}: {.val {field_names}}.",
+          "i" = "Column name{?s}: {.val {col_names}}."
         ),
         class = "frictionless_error_fields_colnames_mismatch"
       )

R/create_schema.R

@@ -72,7 +72,7 @@ create_schema <- function(data) {
   # Columns with all NA are considered logical by R (and read_delim)
   # Set those to character, since string is a better default for Table Schema
   data_as_list <- lapply(data, function(x) {
-    if (is.logical(x) & all(is.na(x))) {
+    if (is.logical(x) && all(is.na(x))) {
       as.character(x)
     } else {
       x
@@ -120,7 +120,7 @@ create_schema <- function(data) {
   # Remove elements that are NULL or empty list
   schema <- clean_list(
     schema,
-    function(x) is.null(x) | length(x) == 0L,
+    function(x) is.null(x) || length(x) == 0L,
     recursive = TRUE
   )
 

R/get_resource.R

@@ -22,28 +22,28 @@ get_resource <- function(package, resource_name) {
 
   # Check resource
   resource_names <- resources(package)
-  assertthat::assert_that(
-    resource_name %in% resource_names,
-    msg = glue::glue(
-      "Can't find resource `{resource_name}` in {resource_names_collapse}.",
-      resource_names_collapse = glue::glue_collapse(
-        glue::backtick(resource_names),
-        sep = ", "
-      )
+  if (!resource_name %in% resources(package)) {
+    cli::cli_abort(
+      c(
+        "Can't find resource {.val {resource_name}} in {.arg package}.",
+        "i" = "Available resource{?s}: {.val {resources(package)}}."
+      ),
+      class = "frictionless_error_resource_not_found"
     )
-  )
+  }
 
   # Get resource
   resource <- purrr::keep(package$resources, ~ .x$name == resource_name)[[1]]
 
   # Check path(s) to file(s)
   # https://specs.frictionlessdata.io/data-resource/#data-location
-  assertthat::assert_that(
-    !is.null(resource$path) | !is.null(resource$data),
-    msg = glue::glue(
-      "Resource `{resource_name}` must have property `path` or `data`."
+  if (is.null(resource$path) && is.null(resource$data)) {
+    cli::cli_abort(
+      "Resource {.val {resource_name}} must have a {.field path} or
+      {.field data} property.",
+      class = "frictionless_error_resource_without_path_data"
     )
-  )
+  }
 
   # Assign read_from property (based on path, then df, then data)
   if (length(resource$path) != 0) {

R/read_package.R

@@ -34,7 +34,7 @@ read_package <- function(file = "datapackage.json") {
   # Check resources
   # https://specs.frictionlessdata.io/data-package/#metadata
   assertthat::assert_that(
-    length(descriptor$resources) != 0 & # Null or empty list
+    length(descriptor$resources) != 0 && # Null or empty list
       purrr::every(descriptor$resources, ~ !is.null(.x$name)),
     msg = glue::glue(
       "Descriptor `{file}` must have property `resources` containing at least",

tests/testthat/test-add_resource.R

@@ -23,20 +23,47 @@ test_that("add_resource() returns error when resource name contains invalid
            characters", {
   p <- example_package
   df <- data.frame("col_1" = c(1, 2), "col_2" = c("a", "b"))
+
+  # Invalid names
+  expect_error(
+    add_resource(p, "New", df),
+    class = "frictionless_error_resource_name_invalid"
+  )
   expect_error(
     add_resource(p, "New", df),
-    paste(
-      "`New` must only contain lowercase alphanumeric characters plus",
-      "`.`, `-` and `_`."
+    regexp = paste(
+      "`resource_name` must only consist of lowercase alphanumeric characters,",
+      "\".\", \"-\" and \"_\"."
     ),
     fixed = TRUE
   )
-  expect_error(add_resource(p, "nëw", df), "only contain lowercase")
-  expect_error(add_resource(p, " new", df), "only contain lowercase")
-  expect_error(add_resource(p, "new ", df), "only contain lowercase")
-  expect_error(add_resource(p, "n ew", df), "only contain lowercase")
-  expect_error(add_resource(p, "n/ew", df), "only contain lowercase")
+  expect_error(
+    add_resource(p, "New", df),
+    regexp = "\"New\" does not meet those criteria.",
+    fixed = TRUE
+  )
+  expect_error(
+    add_resource(p, "nëw", df),
+    class = "frictionless_error_resource_name_invalid"
+  )
+  expect_error(
+    add_resource(p, " new", df),
+    class = "frictionless_error_resource_name_invalid"
+  )
+  expect_error(
+    add_resource(p, "new ", df),
+    class = "frictionless_error_resource_name_invalid"
+  )
+  expect_error(
+    add_resource(p, "n ew", df),
+    class = "frictionless_error_resource_name_invalid"
+  )
+  expect_error(
+    add_resource(p, "n/ew", df),
+    class = "frictionless_error_resource_name_invalid"
+  )
 
+  # Valid names
   expect_true(check_package(add_resource(p, "n.ew", df)))
   expect_true(check_package(add_resource(p, "n-ew", df)))
   expect_true(check_package(add_resource(p, "n_ew", df)))
@@ -50,7 +77,11 @@ test_that("add_resource() returns error when resource of that name already
   df <- data.frame("col_1" = c(1, 2), "col_2" = c("a", "b"))
   expect_error(
     add_resource(p, "deployments", df),
-    "`package` already contains a resource named `deployments`.",
+    class = "frictionless_error_resource_already_exists"
+  )
+  expect_error(
+    add_resource(p, "deployments", df),
+    regexp = "`package` already contains a resource named \"deployments\".",
     fixed = TRUE
   )
 })
@@ -60,7 +91,11 @@ test_that("add_resource() returns error when data is not data frame or
   p <- example_package
   expect_error(
     add_resource(p, "new", list()),
-    "`data` must be a data frame or path(s) to CSV file(s).",
+    class = "frictionless_error_data_type_invalid"
+  )
+  expect_error(
+    add_resource(p, "new", list()),
+    regexp = "`data` must either be a data frame or path(s) to CSV file(s).",
     fixed = TRUE
   )
 })
@@ -71,13 +106,11 @@ test_that("add_resource() returns error on invalid or empty data frame", {
   schema <- create_schema(df)
   expect_error(
     add_resource(p, "new", data.frame("col_1" = character(0))),
-    "`data` must be a data frame containing data.",
-    fixed = TRUE
+    class = "frictionless_error_data_invalid"
   )
   expect_error(
     add_resource(p, "new", data.frame("col_1" = character(0)), schema),
-    "`data` must be a data frame containing data.",
-    fixed = TRUE
+    class = "frictionless_error_data_invalid"
   )
 
   # For more tests see test-check_schema.R
@@ -140,6 +173,10 @@ test_that("add_resource() returns error if ... arguments are unnamed", {
   p <- create_package()
   df <- data.frame("col_1" = c(1, 2), "col_2" = c("a", "b"))
   schema <- create_schema(df)
+  expect_error(
+    add_resource(p, "new", df, schema, delim = ",", "unnamed_value"),
+    class = "frictionless_error_unnamed_argument"
+  )
   expect_error(
     add_resource(p, "new", df, schema, delim = ",", "unnamed_value"),
     "All arguments in `...` must be named.",
@@ -152,18 +189,31 @@ test_that("add_resource() returns error if ... arguments are reserved", {
   df <- data.frame("col_1" = c(1, 2), "col_2" = c("a", "b"))
   expect_error(
     add_resource(p, "new", df, name = "custom_name"),
-    paste(
-      "`name` must be removed as an argument.",
-      "It is automatically added as a resource property by the function."
-    ),
+    class = "frictionless_error_resource_properties_reserved"
+  )
+  expect_error(
+    add_resource(p, "new", df, name = "custom_name", format = "custom_format"),
+    class = "frictionless_error_resource_properties_reserved"
+  )
+
+  expect_error(
+    add_resource(p, "new", df, name = "custom_name"),
+    regexp = "`name` must be removed as argument.",
     fixed = TRUE
   )
   expect_error(
-    add_resource(p, "new", df, path = "custom_path", encoding = "utf8"),
-    paste(
-      "`path` must be removed as an argument.", # First conflicting argument
-      "It is automatically added as a resource property by the function."
-    ),
+    add_resource(p, "new", df, name = "custom_name"),
+    regexp = "name is automatically added as resource property.",
+    fixed = TRUE
+  )
+  expect_error(
+    add_resource(p, "new", df, name = "custom_name", format = "custom_format"),
+    regexp = "`name` and `format` must be removed as arguments.",
+    fixed = TRUE
+  )
+  expect_error(
+    add_resource(p, "new", df, name = "custom_name", format = "custom_format"),
+    regexp = "name and format are automatically added as resource properties.",
     fixed = TRUE
   )
 })

tests/testthat/test-check_schema.R

@@ -80,7 +80,7 @@ test_that("check_schema() returns error when Table Schema fields have invalid ty
   )
   expect_error(
     check_schema(invalid_schema),
-    regexp = "All fields in `schema` must have a valid type.",
+    regexp = "All fields in `schema` must have a valid type property.",
     fixed = TRUE
   )
   expect_error(
@@ -149,12 +149,12 @@ test_that("check_schema() returns error on mismatching schema and data frame", {
   )
   expect_error(
     check_schema(invalid_schema, df),
-    regexp = "Field names: \"col_2\" and \"col_1\"",
+    regexp = "Field names: \"col_2\" and \"col_1\".",
     fixed = TRUE
   )
   expect_error(
     check_schema(invalid_schema, df),
-    regexp = "Column names: \"col_1\" and \"col_2\"", # Same for other tests
+    regexp = "Column names: \"col_1\" and \"col_2\".", # Same for other tests
     fixed = TRUE
   )
 
@@ -184,7 +184,7 @@ test_that("check_schema() returns error on mismatching schema and data frame", {
   )
   expect_error(
     check_schema(invalid_schema, df),
-    regexp = "Field names: \"col_1\", \"col_2\", and \"col_3\"",
+    regexp = "Field names: \"col_1\", \"col_2\", and \"col_3\".",
     fixed = TRUE
   )
 })

tests/testthat/test-read_resource.R

@@ -109,11 +109,7 @@ test_that("read_resource() returns error on invalid resource", {
   # No such resource
   expect_error(
     read_resource(p, "no_such_resource"),
-    paste(
-      "Can't find resource `no_such_resource` in `deployments`,",
-      "`observations`, `media`."
-    ),
-    fixed = TRUE
+    class = "frictionless_error_resource_not_found"
   )
 
   # Create invalid package and add properties one by one to pass errors
@@ -123,7 +119,11 @@ test_that("read_resource() returns error on invalid resource", {
   # No path or data
   expect_error(
     read_resource(p_invalid, "deployments"),
-    "Resource `deployments` must have property `path` or `data`.",
+    class = "frictionless_error_resource_without_path_data"
+  )
+  expect_error(
+    read_resource(p_invalid, "deployments"),
+    regexp = "Resource \"deployments\" must have a path or data property.",
     fixed = TRUE
   )