diff --git a/NAMESPACE b/NAMESPACE index 6624d0b6..56d10a09 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -33,6 +33,7 @@ export(backquote) export(contains_vars) export(convert_dtm_to_dtc) export(dataset_vignette) +export(deprecate_inform) export(dquote) export(enumerate) export(expect_dfs_equal) @@ -96,6 +97,7 @@ importFrom(dplyr,ungroup) importFrom(dplyr,union) importFrom(glue,glue) importFrom(glue,glue_collapse) +importFrom(lifecycle,deprecate_soft) importFrom(lifecycle,deprecate_stop) importFrom(lifecycle,deprecate_warn) importFrom(lifecycle,deprecated) diff --git a/NEWS.md b/NEWS.md index a9e0c55b..046ab884 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,10 @@ # admiraldev (development version) +## New Features + +- New Function `deprecate_inform()` is a wrapper around + `lifecycle::deprecate_soft()` to allow for more control over messaging. (#466) + ## Updates of Existing Functions - Objects passed to `assert_list_element()` via the `...` argument can now be @@ -27,6 +32,7 @@ check whether the vector has a specific length. (#473) - Removed at v1.0.0 `assert_function_param()` ## Documentation + - Deprecation Strategy updated for the long haul! (#466) ## Other diff --git a/R/admiraldev-package.R b/R/admiraldev-package.R index a478d0a9..76b4a51d 100644 --- a/R/admiraldev-package.R +++ b/R/admiraldev-package.R @@ -20,7 +20,7 @@ #' time_length %--% ymd ymd_hms weeks years hours minutes #' @importFrom tidyr drop_na nest pivot_longer pivot_wider unnest #' @importFrom tidyselect all_of contains vars_select -#' @importFrom lifecycle deprecate_warn deprecated deprecate_stop +#' @importFrom lifecycle deprecate_warn deprecated deprecate_stop deprecate_soft #' @importFrom cli cli_abort cli_div #' @importFrom glue glue glue_collapse "_PACKAGE" diff --git a/R/lifecycle_admiral.R b/R/lifecycle_admiral.R new file mode 100644 index 00000000..fa9c6970 --- /dev/null +++ b/R/lifecycle_admiral.R @@ -0,0 +1,50 @@ +#' Deprecation with Soft Message +#' +#' Wrapper around `lifecycle::deprecate_soft()` that messages users about +#' deprecated features and functions instead of warning. +#' +#' @inheritParams lifecycle::deprecate_soft +#' +#' @return `NULL`, invisibly. +#' +#' @examples +#' # A Phase 1 deprecated function with custom bulleted list: +#' deprecate_inform( +#' when = "1.0.0", +#' what = "foo()", +#' details = c( +#' x = "This message will turn into a warning with release of x.y.z", +#' i = "See admiral's deprecation guidance: +#' https://pharmaverse.github.io/admiraldev/dev/articles/programming_strategy.html#deprecation" +#' ) +#' ) +#' +#' @keywords messages +#' @family messages +#' +#' +#' @export +deprecate_inform <- function( + when, + what, + with = NULL, + details = NULL, + id = NULL, + env = rlang::caller_env(), + user_env = rlang::caller_env(2)) { + tryCatch( + lifecycle::deprecate_soft( + when = when, + what = what, + with = with, + details = details, + id = id, + env = env, + user_env = user_env + ), + warning = \(w) { + message(conditionMessage(w)) + tryInvokeRestart("muffleWarning") + } + ) +} diff --git a/_pkgdown.yml b/_pkgdown.yml index 7d62bcf6..367c0667 100644 --- a/_pkgdown.yml +++ b/_pkgdown.yml @@ -88,16 +88,23 @@ reference: desc: Utilities without a theme contents: - has_keyword('dev_utility') + - has_keyword('messages') - title: Deprecated desc: | - As `{admiral}` is still evolving, functions/arguments may need to be removed or replaced over time. In such cases, the function/argument will enter the following 6-month deprecation cycle: + Functions and arguments may need to be removed or replaced over time. + In such cases, the function or argument will enter a 3 year deprecation cycle. + The cycle will be tied as close as sensibly possible to a package release. - * In the first release (0-3 months), there will be a warning issued if you use the function/argument, but it will still be available to use. - * In the following release (3-6 months), an error will be produced if you use the function/argument. - * Finally, from the 3rd release (6 months) onwards, the function/argument will be removed from `{admiral}` and its documentation completely. + When a function is deprecated: - *Note: Guidance on replacement functionality can be found in the warning/error message produced or in the function's documentation.* + * In Year 1 there will be a message issued if you use the function/argument, but it will still be available to use. + * In Year 2 a warning will be produced if you use the function/argument, but it will still be available to use. + * In Year 3, an error will be produced if you use the function/argument and no longer be able to use. + * Finally, after 3 years, the function/argument and related documentation and tests will be removed from `{admiral}`. + + *Note: Guidance on replacement functionality will be found in the message produced as well as in the function's + documentation* Below, you can find a list of functions in the process of being deprecated: contents: diff --git a/inst/WORDLIST b/inst/WORDLIST index 450a8236..f6b4f8cd 100644 --- a/inst/WORDLIST +++ b/inst/WORDLIST @@ -32,8 +32,8 @@ Modularity Mubasheer Neitmann OCCDS -Ondrej ORCID +Ondrej PHUSE PRs Pharma @@ -45,12 +45,12 @@ README RStudio Rimler Roxygen -Samia SDTM +Samia Slama Syed TAs -Thomas +TBD TOC TTE USUBJID @@ -65,7 +65,7 @@ admiraltemplate admiralxxx advs anonymized -arg +behaviour cmd codebase codeowner @@ -94,6 +94,7 @@ pharmaverse pkgs pre proc +programmatically quosures repo reproducibility diff --git a/man/deprecate_inform.Rd b/man/deprecate_inform.Rd new file mode 100644 index 00000000..7c00fe9c --- /dev/null +++ b/man/deprecate_inform.Rd @@ -0,0 +1,80 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/lifecycle_admiral.R +\name{deprecate_inform} +\alias{deprecate_inform} +\title{Deprecation with Soft Message} +\usage{ +deprecate_inform( + when, + what, + with = NULL, + details = NULL, + id = NULL, + env = rlang::caller_env(), + user_env = rlang::caller_env(2) +) +} +\arguments{ +\item{when}{A string giving the version when the behaviour was deprecated.} + +\item{what}{A string describing what is deprecated: +\itemize{ +\item Deprecate a whole function with \code{"foo()"}. +\item Deprecate an argument with \code{"foo(arg)"}. +\item Partially deprecate an argument with +\code{"foo(arg = 'must be a scalar integer')"}. +\item Deprecate anything else with a custom message by wrapping it in \code{I()}. +} + +You can optionally supply the namespace: \code{"ns::foo()"}, but this is +usually not needed as it will be inferred from the caller environment.} + +\item{with}{An optional string giving a recommended replacement for the +deprecated behaviour. This takes the same form as \code{what}.} + +\item{details}{In most cases the deprecation message can be +automatically generated from \code{with}. When it can't, use \code{details} +to provide a hand-written message. + +\code{details} can either be a single string or a character vector, +which will be converted to a \link[cli:cli_bullets]{bulleted list}. +By default, info bullets are used. Provide a named vectors to +override.} + +\item{id}{The id of the deprecation. A warning is issued only once +for each \code{id}. Defaults to the generated message, but you should +give a unique ID when the message in \code{details} is built +programmatically and depends on inputs, or when you'd like to +deprecate multiple functions but warn only once for all of them.} + +\item{env, user_env}{Pair of environments that define where \verb{deprecate_*()} +was called (used to determine the package name) and where the function +called the deprecating function was called (used to determine if +\code{deprecate_soft()} should message). + +These are only needed if you're calling \verb{deprecate_*()} from an internal +helper, in which case you should forward \code{env = caller_env()} and +\code{user_env = caller_env(2)}.} +} +\value{ +\code{NULL}, invisibly. +} +\description{ +Wrapper around \code{lifecycle::deprecate_soft()} that messages users about +deprecated features and functions instead of warning. +} +\examples{ +# A Phase 1 deprecated function with custom bulleted list: +deprecate_inform( + when = "1.0.0", + what = "foo()", + details = c( + x = "This message will turn into a warning with release of x.y.z", + i = "See admiral's deprecation guidance: +https://pharmaverse.github.io/admiraldev/dev/articles/programming_strategy.html#deprecation" + ) +) + +} +\concept{messages} +\keyword{messages} diff --git a/tests/testthat/_snaps/lifecycle_admiral.md b/tests/testthat/_snaps/lifecycle_admiral.md new file mode 100644 index 00000000..01cd2a7d --- /dev/null +++ b/tests/testthat/_snaps/lifecycle_admiral.md @@ -0,0 +1,30 @@ +# lifecycle_admiral Test 1: Simple message is sent to user + + Code + example_fun(data) + Message + `example_fun()` was deprecated in admiraldev 1.0.0. + i Please use `example_fun2()` instead. + Code + example_fun(data) + Message + `example_fun()` was deprecated in admiraldev 1.0.0. + i Please use `example_fun2()` instead. + +# lifecycle_admiral Test 2: Spicier message is sent to user + + Code + example_fun(data) + Message + `example_fun()` was deprecated in admiraldev 1.0.0. + i Please use `example_fun2()` instead. + x This message will turn into a warning with release of 1.1.0 + i See admiral's deprecation guidance: https://pharmaverse.github.io/admiraldev/dev/articles/programming_strategy.html#deprecation + Code + example_fun(data) + Message + `example_fun()` was deprecated in admiraldev 1.0.0. + i Please use `example_fun2()` instead. + x This message will turn into a warning with release of 1.1.0 + i See admiral's deprecation guidance: https://pharmaverse.github.io/admiraldev/dev/articles/programming_strategy.html#deprecation + diff --git a/tests/testthat/test-lifecycle_admiral.R b/tests/testthat/test-lifecycle_admiral.R new file mode 100644 index 00000000..9be7c7af --- /dev/null +++ b/tests/testthat/test-lifecycle_admiral.R @@ -0,0 +1,50 @@ +## Test 1: Message is sent to user ---- +test_that("lifecycle_admiral Test 1: Simple message is sent to user", { + example_fun <- function(dataset) { + deprecate_inform( + when = "1.0.0", + what = "example_fun()", + with = "example_fun2()" + ) + assert_data_frame(dataset, required_vars = exprs(STUDYID, USUBJID)) + } + + data <- dplyr::tribble( + ~STUDYID, ~USUBJID, + "xyz", 123, + "xyz", 456 + ) + + expect_snapshot({ + example_fun(data) + example_fun(data) + }) +}) + +# Test 2: Nicer message is sent to user ---- +test_that("lifecycle_admiral Test 2: Spicier message is sent to user", { + example_fun <- function(dataset) { + deprecate_inform( + when = "1.0.0", + what = "example_fun()", + with = "example_fun2()", + details = c( + x = "This message will turn into a warning with release of 1.1.0", + i = "See admiral's deprecation guidance: + https://pharmaverse.github.io/admiraldev/dev/articles/programming_strategy.html#deprecation" # nolint + ) + ) + assert_data_frame(dataset, required_vars = exprs(STUDYID, USUBJID)) + } + + data <- dplyr::tribble( + ~STUDYID, ~USUBJID, + "xyz", 123, + "xyz", 456 + ) + + expect_snapshot({ + example_fun(data) + example_fun(data) + }) +}) diff --git a/vignettes/programming_strategy.Rmd b/vignettes/programming_strategy.Rmd index 1590892d..8f384b9d 100644 --- a/vignettes/programming_strategy.Rmd +++ b/vignettes/programming_strategy.Rmd @@ -559,25 +559,33 @@ See [Writing Unit Tests in {admiral}](unit_test_guidance.html#writing-unit-tests # Deprecation -As `{admiral}` is still evolving, functions or arguments may need to be removed -or replaced with more efficient options from one release to another. In such -cases, the relevant function or argument must be marked as deprecated. This -deprecation is done in three phases over our release cycles. +The below deprecation strategy provides stability to users while allowing admiral developers +the ability to remove and update the codebase in the coming days. -- **Phase 1:** In the release where the identified function or argument is to -be deprecated there will be a warning issued when using the function or argument -using `deprecate_warn()`. +- **Phase 1:** In the release where the identified function or argument is to +be deprecated, there will be a message issued when using the function or argument +using `deprecate_inform()`. This message will appear to the user for _one year_. The +message will include the a recommendation on which function or argument to change +to in their code. -- **Phase 2:** In the next release an error will be thrown using -`deprecate_stop()`. +- **Phase 2:** After _one year_ and in the closet next release, a warning will be +issued when using the function or argument using `deprecate_warn()`. This warning +message will appear to the user for _one year_. The message a recommendation on which +function or argument to change to in their code. -- **Phase 3:** Finally in the 3rd release thereafter the function will be -removed from the package altogether. +- **Phase 3:** After _one year_ and in the closest next release, an error will be thrown +when using the function or argument using `deprecate_stop()` and follow similar process +for Phase 1 and Phase 2. -Information about deprecation timelines must be added to the warning/error message. +- **Phase 4:** Finally after three years from the time of being identified for deprecation, the +function or argument will be completely removed from `{admiral}`. -Note that the deprecation cycle time for a function or argument based on our -current release schedule is 6 months. +**NB:** Major/Minor release make the most sense for deprecation updates. However, if +a release cycle becomes multiple years, then patch releases should be considered to +help keep `{admiral}` neat and tidy! + +**NB:** Take care with the `NEWS.md` entries around deprecation as the person continuing this +process might not be you! ## Documentation @@ -585,10 +593,10 @@ If a function or argument is removed, the documentation must be updated to indicate the function or the argument is now deprecated and which new function/argument should be used instead. -The documentation will be updated at: +The documentation will be updated at Phase 2: + the description level for a function, -+ the `@keywords` and`@family` roxygen tags will be replaced with `deprecated` ++ the `@keywords` and `@family` roxygen tags will be replaced with `deprecated` ```{r, eval=FALSE} #' Title of the function @@ -599,39 +607,61 @@ The documentation will be updated at: #' This function is *deprecated*, please use `new_fun()` instead. #' . #' @family deprecated -#' #' @keywords deprecated -#' . ``` -+ the `@examples` section should be removed. +Example for documentation at the argument level + the `@param` level for a argument. - ``` - @param old_param *Deprecated*, please use `new_param` instead. - ``` + ``` + @param old_param `r lifecycle::badge("deprecated")` Please use `new_param` instead. + ``` + +The documentation will be further updated at Phase 3: + ++ the `@examples` section should be removed. -## Handling of Warning and Error +## Handling of Messages, Warnings and Errors When a function or argument is deprecated, the function must be updated to issue -a warning or error using `deprecate_warn()` and `deprecate_stop()`, -respectively, as described above. +a message, warning or error using `deprecate_inform()`, `deprecate_warn()` or +`deprecate_stop()`, respectively, as described above. There should be a test case added in the test file of the function that checks -whether this warning/error is issued as appropriate when using the deprecated +whether this message/warning/error is issued as appropriate when using the deprecated function or argument. ### Function -In the initial release in which a function is deprecated the original function -body must be replaced with a call to `deprecate_warn()` and subsequently all +**Phase 1:** In the initial release in which a function is deprecated the original function +body must be replaced with a call to `deprecate_inform()` and subsequently all arguments should be passed on to the new function. +```r +fun_xxx <- function(dataset, some_param, other_param) { + deprecate_inform( + when = "x.y.z", + what = "fun_xxx()", + with = "new_fun_xxx()", + details = c( + x = "This message will turn into a warning with release of x.y.z", + i = "See admiral's deprecation guidance: + https://pharmaverse.github.io/admiraldev/dev/articles/programming_strategy.html#deprecation" + ) + ) + dataset = dataset, + some_param = some_param, + other_param = other_param +} +``` +**Phase 2:** In the next year/closest release in which a function is deprecated +the call to `deprecate_inform()` must be replaced with a call to `deprecate_warn()`. + ```r fun_xxx <- function(dataset, some_param, other_param) { deprecate_warn("x.y.z", "fun_xxx()", "new_fun_xxx()") - new_fun_xxx( + fun_xxx( dataset = dataset, some_param = some_param, other_param = other_param @@ -639,7 +669,7 @@ fun_xxx <- function(dataset, some_param, other_param) { } ``` -In the following release the function body should be changed to just include a call +**Phase 3:** In the following release the function body should be changed to just include a call to `deprecate_stop()`. ```r @@ -648,21 +678,28 @@ fun_xxx <- function(dataset, some_param, other_param) { } ``` -Finally, in the next release the function should be removed from the package. +**Phase 4:** Finally, in the next release the function should be removed from the package. ### Argument -If an argument is removed and is not replaced, an **error** must be generated: +**Phase 1:** If the argument is renamed or replaced, a **message** must be issued and the +new argument takes the value of the old argument until the next release. Note: +arguments which are not passed as `exprs()` argument (e.g. `new_var = VAR1` or +`filter = AVAL >10`) will need to be quoted. -``` +``` ### BEGIN DEPRECATION if (!missing(old_param)) { - deprecate_stop("x.y.z", "fun_xxx(old_param = )", "fun_xxx(new_param = )") + deprecate_inform("x.y.z", "fun_xxx(old_param = )", "fun_xxx(new_param = )") + # old_param is given using exprs() + new_param <- old_param + # old_param is NOT given using exprs() + new_param <- enexpr(old_param) } ### END DEPRECATION ``` -If the argument is renamed or replaced, a **warning** must be issued and the +**Phase 2:** If the argument is renamed or replaced, a **warning** must be issued and the new argument takes the value of the old argument until the next release. Note: arguments which are not passed as `exprs()` argument (e.g. `new_var = VAR1` or `filter = AVAL >10`) will need to be quoted. @@ -679,9 +716,22 @@ arguments which are not passed as `exprs()` argument (e.g. `new_var = VAR1` or ### END DEPRECATION ``` +**Phase 3:** If an argument is removed and is not replaced, an **error** must be generated: + +``` +### BEGIN DEPRECATION + if (!missing(old_param)) { + deprecate_stop("x.y.z", "fun_xxx(old_param = )", "fun_xxx(new_param = )") + } +### END DEPRECATION +``` + + + ## Unit Testing -Unit tests for deprecated functions and arguments must be added to the test file [^1] of the function to ensure that a warning or error is issued. +Unit tests for deprecated functions and arguments must be added to the test +file [^1] of the function to ensure that a message, warning, or error is issued. [^1]: For example, if `derive_var_example()` is going to be deprecated and it is defined in `examples.R`, the unit tests are in @@ -694,6 +744,11 @@ respectively. The unit-test should follow the corresponding format, per the ### For Deprecated Functions that Issues a Warning (Phase 1) +TBD + + +### For Deprecated Functions that Issues a Warning (Phase 2) + A unit test like the following must be added. ``` ## Test #: deprecation warning if function is called ---- @@ -715,7 +770,7 @@ In the existing unit tests the call of the deprecated function need to be enclos The `regexpr` argument must be specified to ensure that only the deprecation warning is suppressed. -### For Deprecated Functions that Issues an Error (Phase 2) +### For Deprecated Functions that Issues an Error (Phase 3) A unit test like the following must be added. ```