From 0fac7c9e1046eaa8394523abc72eaf62a33dd86f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 20 Sep 2022 16:17:33 -0500 Subject: [PATCH 01/10] Restore desired soft deprecation behaviour --- NEWS.md | 7 +++++++ R/deprecated.R | 27 +++++++++++++++++++++------ tests/testthat/test-deprecated.R | 3 +-- tests/testthat/test-lifecycle.R | 13 +++++++++---- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/NEWS.md b/NEWS.md index 407abf2..e217458 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,12 @@ # lifecycle (development version) +* `deprecate_soft()` now only deprecates in tests if the deprecation is + you are not on CRAN and the function comes from the current package. + This ensures that soft deprecation never breaks `R CMD check` on CRAN + and is only ever signalled when you can do something about it (i.e. + when the deprecation comes from your package, not one of your + dependencies). + # lifecycle 1.0.2 * You can now generate arbitrary text in a deprecation message by diff --git a/R/deprecated.R b/R/deprecated.R index 7d6e7c3..a223294 100644 --- a/R/deprecated.R +++ b/R/deprecated.R @@ -102,14 +102,15 @@ deprecate_soft <- function(when, verbosity <- lifecycle_verbosity() if (verbosity == "quiet") { NULL - } else if (verbosity %in% "warning" || - (is_string(verbosity, "default") && env_inherits_global(user_env))) { - trace <- trace_back(bottom = caller_env()) - deprecate_warn0(msg, trace, always = TRUE) + } else if (verbosity %in% c("warning", "default")) { + if (env_inherits_global(user_env) || from_testthat(user_env)) { + trace <- trace_back(bottom = caller_env()) + deprecate_warn0(msg, trace, always = TRUE) + } else { + deprecate_soft0(msg) + } } else if (verbosity == "error") { deprecate_stop0(msg) - } else { - deprecate_soft0(msg) } invisible(NULL) @@ -306,6 +307,20 @@ env_inherits_global <- function(env) { is_reference(topenv(env), global_env()) } +# TRUE if we are in unit tests and the package being tested is the +# same as the package that called +from_testthat <- function(env) { + tested_package <- Sys.getenv("TESTTHAT_PKG") + + # browser() + + # Test for environment names rather than reference/contents because + # testthat clones the namespace + nzchar(tested_package) && + identical(Sys.getenv("NOT_CRAN"), "true") && + env_name(topenv(env)) == paste0("namespace:", tested_package) +} + needs_warning <- function(id) { if (!is_string(id)) { lifecycle_abort("`id` must be a string") diff --git a/tests/testthat/test-deprecated.R b/tests/testthat/test-deprecated.R index 321fd0e..0dd1fd1 100644 --- a/tests/testthat/test-deprecated.R +++ b/tests/testthat/test-deprecated.R @@ -40,7 +40,6 @@ test_that("quiet suppresses _soft and _warn", { test_that("warning always warns in _soft and _warn", { local_options(lifecycle_verbosity = "warning") - expect_deprecated(deprecate_soft("1.0.0", "foo()")) expect_deprecated(deprecate_warn("1.0.0", "foo()")) expect_defunct(deprecate_stop("1.0.0", "foo()")) }) @@ -54,7 +53,7 @@ test_that("error coverts _soft and _warn to errors", { }) test_that("soft deprecation uses correct calling envs", { - local_options(lifecycle_verbosity = "default") + withr::local_envvar(TESTTHAT_PKG = "testpackage") # Simulate package functions available from global environment env <- new_environment(parent = ns_env("lifecycle")) diff --git a/tests/testthat/test-lifecycle.R b/tests/testthat/test-lifecycle.R index e8a361b..6b2df11 100644 --- a/tests/testthat/test-lifecycle.R +++ b/tests/testthat/test-lifecycle.R @@ -1,5 +1,5 @@ test_that("deprecate_soft() warns when called from global env", { - local_options(lifecycle_verbosity = NULL) + withr::local_envvar(TESTTHAT_PKG = "testpackage") fn <- function(id) { deprecate_soft("1.0.0", "foo()", id = id) @@ -79,10 +79,15 @@ test_that("the topenv of the empty env is not the global env", { }) test_that("expect_deprecated() matches regexp", { - expect_deprecated(deprecate_soft("1.0", "fn()", details = "foo"), "foo") - expect_deprecated(deprecate_warn("1.0", "fn()", details = "foo.["), "foo.[", fixed = TRUE) + expect_deprecated( + deprecate_warn("1.0", "fn()", details = "foo.["), "foo.[", fixed = TRUE + ) + fn <- function(id) { + deprecate_soft("1.0.0", "fn()", id = id) + } + expect_deprecated(fn(), "fn") expect_deprecated(expect_failure( - expect_deprecated(deprecate_soft("1.0", "fn()"), "foo") + expect_deprecated(fn(), "foo") )) }) From 8aa8bc96ecba2be21cbc4f2004040374930ced2b Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 20 Sep 2022 16:23:27 -0500 Subject: [PATCH 02/10] Tweak warning behaviour and option --- NEWS.md | 2 ++ R/deprecated.R | 2 +- R/verbosity.R | 8 ++++---- man/expect_deprecated.Rd | 4 ++-- man/verbosity.Rd | 7 ++++--- 5 files changed, 13 insertions(+), 10 deletions(-) diff --git a/NEWS.md b/NEWS.md index e217458..963caa8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,6 +7,8 @@ when the deprecation comes from your package, not one of your dependencies). +* Soft deprecations now only warn every 8 hours in non-package code. + # lifecycle 1.0.2 * You can now generate arbitrary text in a deprecation message by diff --git a/R/deprecated.R b/R/deprecated.R index a223294..59551ce 100644 --- a/R/deprecated.R +++ b/R/deprecated.R @@ -105,7 +105,7 @@ deprecate_soft <- function(when, } else if (verbosity %in% c("warning", "default")) { if (env_inherits_global(user_env) || from_testthat(user_env)) { trace <- trace_back(bottom = caller_env()) - deprecate_warn0(msg, trace, always = TRUE) + deprecate_warn0(msg, trace, always = verbosity == "warning") } else { deprecate_soft0(msg) } diff --git a/R/verbosity.R b/R/verbosity.R index 2bcf035..37dbdd4 100644 --- a/R/verbosity.R +++ b/R/verbosity.R @@ -13,10 +13,10 @@ #' You can control the level of verbosity with the global option #' `lifecycle_verbosity`. It can be set to: #' -#' * `"default"` or `NULL` for the default non-disruptive settings. -#' -#' * `"quiet"`, `"warning"` or `"error"` to force silence, warnings or -#' errors for deprecated functions. +#' * `"quiet"` to suppress all deprecation messages. +#' * `"default"` or `NULL` to warn once every 8 hours. +#' * `"warning"` to warn every time. +#' * `"error"` to error instead of warning. #' #' Note that functions calling [deprecate_stop()] invariably throw #' errors. diff --git a/man/expect_deprecated.Rd b/man/expect_deprecated.Rd index bd2c04e..9987a1c 100644 --- a/man/expect_deprecated.Rd +++ b/man/expect_deprecated.Rd @@ -19,9 +19,9 @@ expected warning message.} \item{...}{ Arguments passed on to \code{\link[testthat:expect_match]{expect_match}} \describe{ + \item{\code{fixed}}{If \code{TRUE}, treats \code{regexp} as a string to be matched exactly +(not a regular expressions). Overrides \code{perl}.} \item{\code{perl}}{logical. Should Perl-compatible regexps be used?} - \item{\code{fixed}}{logical. If \code{TRUE}, \code{pattern} is a string to be - matched as is. Overrides all conflicting arguments.} }} } \description{ diff --git a/man/verbosity.Rd b/man/verbosity.Rd index 52e3489..aaee23b 100644 --- a/man/verbosity.Rd +++ b/man/verbosity.Rd @@ -15,9 +15,10 @@ and whether the warning was already issued (see the help for You can control the level of verbosity with the global option \code{lifecycle_verbosity}. It can be set to: \itemize{ -\item \code{"default"} or \code{NULL} for the default non-disruptive settings. -\item \code{"quiet"}, \code{"warning"} or \code{"error"} to force silence, warnings or -errors for deprecated functions. +\item \code{"quiet"} to suppress all deprecation messages. +\item \code{"default"} or \code{NULL} to warn once every 8 hours. +\item \code{"warning"} to warn every time. +\item \code{"error"} to error instead of warning. } Note that functions calling \code{\link[=deprecate_stop]{deprecate_stop()}} invariably throw From 60eeb4a32468acba4cf8df1a73d8a1dfc61c259e Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 20 Sep 2022 16:23:46 -0500 Subject: [PATCH 03/10] Add missing withr dep --- DESCRIPTION | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index ef27c43..e3e347c 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -28,7 +28,8 @@ Suggests: tibble, tidyverse, tools, - vctrs + vctrs, + withr VignetteBuilder: knitr Config/testthat/edition: 3 From b0216dc9b4de1b176d877270ae0883a5b814d9d9 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 09:11:33 -0500 Subject: [PATCH 04/10] Don't signal lifecycle_soft_deprecated --- R/deprecated.R | 6 ------ tests/testthat/test-deprecated.R | 8 +++----- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/R/deprecated.R b/R/deprecated.R index 59551ce..830714a 100644 --- a/R/deprecated.R +++ b/R/deprecated.R @@ -106,8 +106,6 @@ deprecate_soft <- function(when, if (env_inherits_global(user_env) || from_testthat(user_env)) { trace <- trace_back(bottom = caller_env()) deprecate_warn0(msg, trace, always = verbosity == "warning") - } else { - deprecate_soft0(msg) } } else if (verbosity == "error") { deprecate_stop0(msg) @@ -170,10 +168,6 @@ deprecate_stop <- function(when, # Signals ----------------------------------------------------------------- -deprecate_soft0 <- function(msg) { - signal(msg, "lifecycle_soft_deprecated") -} - deprecate_warn0 <- function(msg, trace = NULL, always = FALSE) { footer <- function(...) { if (is_interactive()) { diff --git a/tests/testthat/test-deprecated.R b/tests/testthat/test-deprecated.R index 0dd1fd1..7dfd49a 100644 --- a/tests/testthat/test-deprecated.R +++ b/tests/testthat/test-deprecated.R @@ -4,7 +4,6 @@ test_that("default deprecations behave as expected", { on.exit(env_unbind(deprecation_env, "test")) local_options(lifecycle_verbosity = "default") - expect_condition(deprecate_soft("1.0.0", "foo()"), class = "lifecycle_soft_deprecated") expect_warning(deprecate_warn("1.0.0", "foo()", id = "test"), class = "lifecycle_warning_deprecated") expect_warning(deprecate_warn("1.0.0", "foo()", id = "test"), NA) expect_defunct(deprecate_stop("1.0.0", "foo()")) @@ -67,15 +66,14 @@ test_that("soft deprecation uses correct calling envs", { }) local_bindings(!!!as.list(env), .env = global_env()) - # Calling package function directly should warning + # Calling package function directly should warn cnd <- catch_cnd(evalq(softly(), global_env()), "warning") expect_s3_class(cnd, class = "lifecycle_warning_deprecated") expect_match(conditionMessage(cnd), "lifecycle") # Calling package function indirectly from global env shouldn't - cnd <- catch_cnd(evalq(softly_softly(), global_env()), "lifecycle_soft_deprecated") - expect_s3_class(cnd, class = "lifecycle_soft_deprecated") - expect_match(conditionMessage(cnd), "lifecycle") + cnd <- catch_cnd(evalq(softly_softly(), global_env()), "warning") + expect_equal(cnd, NULL) }) test_that("warning conditions are signaled only once if warnings are suppressed", { From d1e653069351e1700ee82de4379d49db8cadd61c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 09:13:32 -0500 Subject: [PATCH 05/10] Refactor deprecate_warn to match --- R/deprecated.R | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/R/deprecated.R b/R/deprecated.R index 830714a..e3b075e 100644 --- a/R/deprecated.R +++ b/R/deprecated.R @@ -133,12 +133,8 @@ deprecate_warn <- function(when, verbosity <- lifecycle_verbosity() if (verbosity == "quiet") { NULL - } else if (verbosity == "warning") { - trace <- trace_back(bottom = caller_env()) - deprecate_warn0(msg, trace, always = TRUE) - } else if (verbosity == "error") { - deprecate_stop0(msg) - } else { + } else if (verbosity %in% c("default", "warning")) { + always <- always || verbosity == "warning" id <- id %||% msg if (always || needs_warning(id)) { @@ -148,6 +144,8 @@ deprecate_warn <- function(when, trace <- trace_back(bottom = caller_env()) deprecate_warn0(msg, trace, always = always) } + } else if (verbosity == "error") { + deprecate_stop0(msg) } invisible(NULL) From af43fcbf41583459e98dd03795459fced81919e6 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 09:14:19 -0500 Subject: [PATCH 06/10] Update NEWS.md Co-authored-by: Lionel Henry --- NEWS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index 963caa8..2a754ee 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,7 +1,7 @@ # lifecycle (development version) -* `deprecate_soft()` now only deprecates in tests if the deprecation is - you are not on CRAN and the function comes from the current package. +* `deprecate_soft()` now only deprecates in tests if you are testing + locally (not on CRAN) and the function comes from the current package. This ensures that soft deprecation never breaks `R CMD check` on CRAN and is only ever signalled when you can do something about it (i.e. when the deprecation comes from your package, not one of your From 2b68706a9b43967d091de41c7c5f30b25b5b5f48 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 09:17:51 -0500 Subject: [PATCH 07/10] Tweak docs --- R/deprecated.R | 8 +++++--- man/deprecate_soft.Rd | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/R/deprecated.R b/R/deprecated.R index e3b075e..b13cde4 100644 --- a/R/deprecated.R +++ b/R/deprecated.R @@ -4,9 +4,11 @@ #' These functions provide three levels of verbosity for deprecated #' functions. Learn how to use them in `vignette("communicate")`. #' -#' * `deprecate_soft()` warns only if the deprecated function is -#' called from the global environment or from the package currently -#' being tested. +#' * `deprecate_soft()` warns only if the deprecated function is called +#' directly, i.e. a user is calling a function they wrote in the global +#' environment or a developer is calling it in their package. It does not +#' warn when called indirectly, i.e. the deprecation comes from code that +#' you don't control. #' #' * `deprecate_warn()` warns unconditionally. #' diff --git a/man/deprecate_soft.Rd b/man/deprecate_soft.Rd index 884e309..01734d7 100644 --- a/man/deprecate_soft.Rd +++ b/man/deprecate_soft.Rd @@ -77,9 +77,11 @@ one release with the default.} These functions provide three levels of verbosity for deprecated functions. Learn how to use them in \code{vignette("communicate")}. \itemize{ -\item \code{deprecate_soft()} warns only if the deprecated function is -called from the global environment or from the package currently -being tested. +\item \code{deprecate_soft()} warns only if the deprecated function is called +directly, i.e. a user is calling a function they wrote in the global +environment or a developer is calling it in their package. It does not +warn when called indirectly, i.e. the deprecation comes from code that +you don't control. \item \code{deprecate_warn()} warns unconditionally. \item \code{deprecate_stop()} fails unconditionally. } From 08faf85b475510b6511221bf7770a151f7328d25 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 09:29:52 -0500 Subject: [PATCH 08/10] Actually only warn every 8 hours --- R/deprecated.R | 24 ++++++++++++------------ tests/testthat/test-lifecycle.R | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/R/deprecated.R b/R/deprecated.R index b13cde4..68e62ea 100644 --- a/R/deprecated.R +++ b/R/deprecated.R @@ -106,8 +106,8 @@ deprecate_soft <- function(when, NULL } else if (verbosity %in% c("warning", "default")) { if (env_inherits_global(user_env) || from_testthat(user_env)) { - trace <- trace_back(bottom = caller_env()) - deprecate_warn0(msg, trace, always = verbosity == "warning") + always <- verbosity == "warning" + deprecate_warn0(msg, id, trace_back(bottom = caller_env()), always = always) } } else if (verbosity == "error") { deprecate_stop0(msg) @@ -137,15 +137,7 @@ deprecate_warn <- function(when, NULL } else if (verbosity %in% c("default", "warning")) { always <- always || verbosity == "warning" - id <- id %||% msg - - if (always || needs_warning(id)) { - # Prevent warning from being displayed again - env_poke(deprecation_env, id, Sys.time()) - - trace <- trace_back(bottom = caller_env()) - deprecate_warn0(msg, trace, always = always) - } + deprecate_warn0(msg, id, trace_back(bottom = caller_env()), always = always) } else if (verbosity == "error") { deprecate_stop0(msg) } @@ -168,7 +160,15 @@ deprecate_stop <- function(when, # Signals ----------------------------------------------------------------- -deprecate_warn0 <- function(msg, trace = NULL, always = FALSE) { +deprecate_warn0 <- function(msg, id = NULL, trace = NULL, always = FALSE) { + id <- id %||% msg + if (!always && !needs_warning(id)) { + return() + } + + # Prevent warning from being displayed again + env_poke(deprecation_env, id, Sys.time()) + footer <- function(...) { if (is_interactive()) { c( diff --git a/tests/testthat/test-lifecycle.R b/tests/testthat/test-lifecycle.R index 6b2df11..79417a6 100644 --- a/tests/testthat/test-lifecycle.R +++ b/tests/testthat/test-lifecycle.R @@ -83,8 +83,8 @@ test_that("expect_deprecated() matches regexp", { deprecate_warn("1.0", "fn()", details = "foo.["), "foo.[", fixed = TRUE ) - fn <- function(id) { - deprecate_soft("1.0.0", "fn()", id = id) + fn <- function() { + deprecate_soft("1.0.0", "fn()") } expect_deprecated(fn(), "fn") expect_deprecated(expect_failure( From 937947ea9ec99c0609ba318dfba040a28249c918 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 09:31:21 -0500 Subject: [PATCH 09/10] Extract out is_direct() function --- R/deprecated.R | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/R/deprecated.R b/R/deprecated.R index 68e62ea..8497959 100644 --- a/R/deprecated.R +++ b/R/deprecated.R @@ -105,7 +105,7 @@ deprecate_soft <- function(when, if (verbosity == "quiet") { NULL } else if (verbosity %in% c("warning", "default")) { - if (env_inherits_global(user_env) || from_testthat(user_env)) { + if (is_direct(user_env)) { always <- verbosity == "warning" deprecate_warn0(msg, id, trace_back(bottom = caller_env()), always = always) } @@ -289,6 +289,10 @@ lifecycle_message_with <- function(with, what) { # Helpers ----------------------------------------------------------------- +is_direct <- function(env) { + env_inherits_global(env) || from_testthat(env) +} + env_inherits_global <- function(env) { # `topenv(emptyenv())` returns the global env. Return `FALSE` in # that case to allow passing the empty env when the @@ -306,8 +310,6 @@ env_inherits_global <- function(env) { from_testthat <- function(env) { tested_package <- Sys.getenv("TESTTHAT_PKG") - # browser() - # Test for environment names rather than reference/contents because # testthat clones the namespace nzchar(tested_package) && From 0deaa74cb11a3385df8df19a9b20d2ecf17ee2cd Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 21 Sep 2022 09:52:55 -0500 Subject: [PATCH 10/10] Polish news --- NEWS.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/NEWS.md b/NEWS.md index 2a754ee..b7053bb 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,13 +1,14 @@ # lifecycle (development version) -* `deprecate_soft()` now only deprecates in tests if you are testing - locally (not on CRAN) and the function comes from the current package. - This ensures that soft deprecation never breaks `R CMD check` on CRAN - and is only ever signalled when you can do something about it (i.e. - when the deprecation comes from your package, not one of your - dependencies). - -* Soft deprecations now only warn every 8 hours in non-package code. +* In tests, `deprecate_soft()` will only warn if the deprecated function + is called directly from the package being tested, not one of its dependencies. + This ensures that you only see the warning when it's your responsibility to + do something about it (#134). + +* `deprecate_soft()` will never warn when called on CRAN, ensuring that soft + deprecation will never break a reverse dependency (#134). + +* Soft deprecations now only warn every 8 hours in non-package code (#134). # lifecycle 1.0.2