Merge pull request #274 from pharmaverse/272_253_72_Deprecation_and_d…

…oc_updates@devel

Closes #272, #253, #72 continues deprecation process and adds some doc updates

NEWS.md

@@ -14,12 +14,20 @@ expressions (#241)
 ## Breaking Changes
 
 - `assert_order_vars()` was deprecated in favor of `assert_expr_list()`. (#241)
+- The following functions have been deprecated from previous admiral versions using the next phase of the deprecation process: (#272)
+
+  - `quo_c()`
+  - `quo_not_missing()`
+  - `replace_symbol_in_quo()`
+- The `quosures` argument was replaced by the `expressions` argument in `replace_values_by_names()`.
 
 ## Documentation
 
 - The deprecation strategy was updated regarding unit tests for deprecated
 functions/arguments in phase 1. (#247)
 
+- The programming strategy was updated regarding permitted values and calling functions from package dependencies (#72, #253)
+
 # admiraldev 0.3.0
 
 ## New Features

R/quo.R

@@ -9,24 +9,20 @@
 #' @return An object of class `quosures`
 #'
 #'
-#' @keywords quo
-#' @family quo
+#' @keywords deprecated
+#' @family deprecated
 #'
 #' @export
 quo_c <- function(...) {
-  deprecate_warn(
-    "0.10.0",
+  deprecate_stop(
+    "0.3.0",
     "quo_c()",
     "expr_c()",
     details = paste(
       "Expressions created by `exprs()` must be used",
       "instead of quosures created by `vars()`."
     )
   )
-  inputs <- unlist(list(...), recursive = TRUE)
-  stopifnot(all(map_lgl(inputs, is_quosure)))
-  is_null <- map_lgl(inputs, quo_is_null)
-  rlang::as_quosures(inputs[!is_null])
 }
 
 #' Concatenate One or More Expressions
@@ -64,12 +60,12 @@ expr_c <- function(...) {
 #' @return TRUE or error.
 #'
 #'
-#' @keywords quo
-#' @family quo
+#' @keywords deprecated
+#' @family deprecated
 #'
 #' @export
 quo_not_missing <- function(x) {
-  deprecate_warn(
+  deprecate_stop(
     "0.3.0",
     "quo_not_missing()",
     details = paste(
@@ -78,15 +74,6 @@ quo_not_missing <- function(x) {
       sep = "\n"
     )
   )
-  !rlang::quo_is_missing(x)
-
-  if (is.null(missing(x)) || quo_is_missing(x)) {
-    stop(paste0(
-      "Argument `",
-      deparse(substitute(x)),
-      "` is missing, with no default"
-    ))
-  }
 }
 
 
@@ -109,8 +96,8 @@ quo_not_missing <- function(x) {
 #' replace_values_by_names(exprs(AVAL, ADT = convert_dtc_to_dt(EXSTDTC)))
 replace_values_by_names <- function(expressions, quosures) {
   if (!missing(quosures)) {
-    deprecate_warn(
-      "0.10.0",
+    deprecate_stop(
+      "0.3.0",
       "replace_values_by_names(quosures = )",
       "replace_values_by_names(expressions = )"
     )
@@ -143,15 +130,15 @@ replace_values_by_names <- function(expressions, quosures) {
 #' @return The quosure where every occurrence of the symbol `target` is replaced
 #'   by `replace`
 #'
-#' @keywords quo
-#' @family quo
+#' @keywords deprecated
+#' @family deprecated
 #'
 #' @export
 replace_symbol_in_quo <- function(quosure,
                                   target,
                                   replace) {
   deprecate_stop(
-    "0.10.0",
+    "0.3.0",
     "replace_symbol_in_quo()",
     "replace_symbol_in_expr()",
     details = paste(

tests/testthat/test-quo.R

@@ -1,9 +1,9 @@
 # quo_c ----
 ## Test 1: issues deprecation warning ----
 test_that("quo_c Test 1: issues deprecation warning", {
-  expect_warning(
+  expect_error(
     quo_c(quo(USUBJID), quo(STUDYID)),
-    class = "lifecycle_warning_deprecated"
+    class = "lifecycle_error_deprecated"
   )
 })
 
@@ -12,13 +12,13 @@ test_that("quo_c Test 2: `quo_c` works in concatenating and indexing quosures",
   x <- quo(USUBJID)
   y <- quo(STUDYID)
 
-  expect_equal(
-    expected = quo(USUBJID),
-    object = quo_c(x, NULL, y)[[1]]
+  expect_error(
+    quo_c(x, NULL, y)[[1]],
+    class = "lifecycle_error_deprecated"
   )
-  expect_equal(
-    expected = quo(STUDYID),
-    object = quo_c(x, NULL, y)[[2]]
+  expect_error(
+    object = quo_c(x, NULL, y)[[2]],
+    class = "lifecycle_error_deprecated"
   )
 })
 
@@ -27,7 +27,8 @@ test_that("quo_c Test 3: `quo_c` returns error if non-quosures are input", {
   USUBJID <- "01-701-1015" # nolint
 
   expect_error(
-    object = quo_c(quo(USUBJID), USUBJID)
+    object = quo_c(quo(USUBJID), USUBJID),
+    class = "lifecycle_error_deprecated"
   )
 })
 
@@ -77,9 +78,9 @@ test_that("quo_not_missing Test 8: issues deprecation warning", {
     x <- enquo(x)
     !isTRUE(quo_not_missing(x))
   }
-  expect_warning(
+  expect_error(
     test_fun(my_variable),
-    class = "lifecycle_warning_deprecated"
+    class = "lifecycle_error_deprecated"
   )
 })
 
@@ -134,9 +135,9 @@ test_that("replace_values_by_names Test 11: names of argument is NULL", {
 
 ## Test 12: warning if quosures argument is used ----
 test_that("replace_values_by_names Test 12: warning if quosures argument is used", {
-  expect_warning(
+  expect_error(
     replace_values_by_names(quosures = rlang::quos(STUDYID, USUBJID)),
-    class = "lifecycle_warning_deprecated"
+    class = "lifecycle_error_deprecated"
   )
 })
 

vignettes/programming_strategy.Rmd

@@ -389,8 +389,12 @@ An example is given below:
 The following fields are mandatory:
 
 * `@param`: One entry per function argument. 
-The following attributes should be described: expected data type (e.g. `data.frame`, `logical`, `numeric` etc.), default value (if any), permitted values (if applicable), optionality (i.e. is this a required argument). 
-If the expected input is a dataset then the required variables should be clearly stated.
+The following attributes should be described: expected data type (e.g. `data.frame`, `logical`, `numeric` etc.), permitted values (if applicable), optionality (i.e. is this a required argument). If the expected input is a dataset then the required variables should be clearly stated. Describing the default value becomes difficult to maintain and subject to manual error when it is already declared in the function arguments. The description for permitted values should be written as a separate line italicizing the phrase "Permitted Values", example below: 
+
+```
+#'   *Permitted Values*: example description of permitted values here
+```
+
 * `@details`: A natural-language description of the derivation used inside the function.
 * `@keyword`: One applicable tag to the function - identical to family.
 * `@family`: One applicable tag to the function - identical to keyword.
@@ -524,6 +528,7 @@ If a package is used only in examples and/or unit tests then it should be listed
 Functions from other packages have to be explicitly imported by using the `@importFrom` tag in the `R/admiral-package.R` file. 
 To import the `if_else()` and `mutate()` function from `dplyr` the following line would have to be included in that file:
 `#' @importFrom dplyr if_else mutate`.
+By using the `@importFrom` tag, it is easier to track all of our dependencies in one place and improves code readability. 
 
 Some of these functions become critically important while using admiral and
 should be included as an export. This applies to functions which are frequently