Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #466 deprecation 20 #467

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
- Removed at v1.0.0 `assert_function_param()`

## Documentation
- Deprecation Strategy updated for the long haul! (#466)

## Other

Expand Down
9 changes: 5 additions & 4 deletions _pkgdown.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,12 @@ reference:

- 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:
As `{admiral}` is still evolving, functions/arguments may need to be removed or replaced over time. In such cases, the function/argument will enter a 3 year deprecation cycle:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this part of the sentence, "As {admiral} is still evolving". All packages are evolving, and admiral isn't different in this way. Previously, it was evolving VERY quickly, so it made sense to include it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could say a minimum of 3 years. We are then not making statements to the users about specific timelines.


* 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.
* In the first release (1 year), there will be a message issued if you use the function/argument, but it will still be available to use.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest to have one year release cycles?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no not really - I will reword this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to include a stage where only direct calls are messaged to users, and indirect calls are messaged to developers? Maybe split this into 6 months messaging to direct calls and devs, and 6 months to any call. This will give devs time to update their package if lower-level functions are updated.

* In the following release (1 year), an warning will be produced if you use the function/argument.
* In the following release (1 year), an error will be produced if you use the function/argument.
* Finally, after 3 years onwards, the function/argument will be removed from `{admiral}` and its documentation completely.

*Note: Guidance on replacement functionality can be found in the warning/error message produced or in the function's documentation.*

Expand Down
101 changes: 72 additions & 29 deletions vignettes/programming_strategy.Rmd
Original file line number Diff line number Diff line change
Expand Up @@ -559,25 +559,35 @@ 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.
`{admiral}` has reached a level of maturity with the release of `1.0.0` in December 2023.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this first sentence. It's top of our mind now, but in 1 year, 2 years, no one will care to read about the 1.0.0 release?

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 date this message will run for and 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 will include the date this
warning message will run for and 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!
Comment on lines +585 to +587
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we should make deprecation updates in patch releases (see https://r-pkgs.org/lifecycle.html#sec-lifecycle-release-type). Do we gain anything by making a patch release instead of a minor release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should discuss at the time of the patch - but I was really thinking far in the future here. if a lot of time has passed between releases (moved away from 1 year releases) and we have a few deprecations to progress and a quick fix for a single function then we probably should just release everything

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A patch release is fine for bumping a function/argument through the deprecation process. You're not making a new breaking change, rather a change in messaging to users about a change that was previously made. The breaking change/deprecation was made when the first deprecation messaging was added.


**NB:** Take care with `NEWS.md` entries for deprecation as the person continuing this
process might not be you!

## Documentation

Expand All @@ -588,7 +598,7 @@ function/argument should be used instead.
The documentation will be updated at:

+ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the dots in line 610 and 614 should be removed.

Expand All @@ -609,25 +619,38 @@ The documentation will be updated at:
+ 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.
```

## 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("x.y.z", "fun_xxx()", "new_fun_xxx()")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to specify the details argument if we want to include the date when the message will turn into a warning.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest not including this much information in the deprecation messages. We've deprecated it, we're messaging the users; including information on when messaging will slightly change something we've already told them to move away from is a bit much. It also locks us into timelines unnecessarily.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm... yeah that makes sense to not lock ourselves in and keep things simple!!

new_fun_xxx(
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()")
Expand All @@ -639,7 +662,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
Expand All @@ -648,21 +671,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.
Expand All @@ -679,9 +709,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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update line 735 to add the class issued by deprecate_inform()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! ACtually I was wondering if we should create deprecate_inform() in this PR and start working on derive_var_dthcaus() or agree and finish the strategy first and then work on the function in a separate PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would implement deprecate_inform() in this PR. Otherwise, we are referring to a function which doesn't exist.

Expand Down
Loading