Closes #466 deprecation 20 #467

bms63 · 2024-10-08T19:36:09Z

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the main branch until you have checked off each task.

github-actions · 2024-10-08T19:37:56Z

Package	Line Rate	Health
admiraldev	97%	✔
Summary	97% (1044 / 1078)	✔

bundfussr · 2024-10-09T12:33:03Z

_pkgdown.yml

-    * 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.


Do you suggest to have one year release cycles?

Ah no not really - I will reword this

vignettes/programming_strategy.Rmd

bundfussr · 2024-10-09T12:51:27Z

vignettes/programming_strategy.Rmd

+**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!


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?

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

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.

vignettes/programming_strategy.Rmd

bundfussr · 2024-10-09T12:58:43Z

vignettes/programming_strategy.Rmd

@@ -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


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

vignettes/programming_strategy.Rmd

bundfussr · 2024-10-09T13:19:01Z

vignettes/programming_strategy.Rmd

 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()")


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

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.

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

vignettes/programming_strategy.Rmd

bundfussr · 2024-10-09T13:30:46Z

vignettes/programming_strategy.Rmd

 ## 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 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


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

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?

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

Co-authored-by: Stefan Bundfuss <[email protected]>

ddsjoberg · 2024-10-09T15:08:05Z

_pkgdown.yml

@@ -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:


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.

ddsjoberg · 2024-10-09T15:10:14Z

_pkgdown.yml

-    * 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.


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.

ddsjoberg · 2024-10-09T15:11:36Z

vignettes/programming_strategy.Rmd

-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.


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?

ddsjoberg · 2024-10-09T15:14:47Z

_pkgdown.yml

@@ -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:


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

ddsjoberg · 2024-10-09T15:17:18Z

vignettes/programming_strategy.Rmd

+**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!


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.

ddsjoberg

Looks great! Just a few comments/suggestions

bms63 added 4 commits October 7, 2024 01:40

docs: #466 deprecate 2.0

ef2f6c5

docs: #466 unit tests and roxygen

5c00f05

chore: #466 news

a78fc84

chore: #466 pkgdown yml

ead0e13

bms63 requested review from ddsjoberg, manciniedoardo and bundfussr October 8, 2024 19:36

bms63 linked an issue Oct 8, 2024 that may be closed by this pull request

Deprecation 2.0 #466

Open

bundfussr reviewed Oct 9, 2024

View reviewed changes

bms63 and others added 6 commits October 9, 2024 10:31

Update vignettes/programming_strategy.Rmd

823ff6c

Co-authored-by: Stefan Bundfuss <[email protected]>

Update vignettes/programming_strategy.Rmd

56eec03

Co-authored-by: Stefan Bundfuss <[email protected]>

Update vignettes/programming_strategy.Rmd

76e4a13

Co-authored-by: Stefan Bundfuss <[email protected]>

Update vignettes/programming_strategy.Rmd

c9304fd

Co-authored-by: Stefan Bundfuss <[email protected]>

Update vignettes/programming_strategy.Rmd

f0d67ef

Co-authored-by: Stefan Bundfuss <[email protected]>

Merge branch 'main' into 466-deprecation-20

9b55526

ddsjoberg reviewed Oct 9, 2024

View reviewed changes

ddsjoberg requested changes Oct 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #466 deprecation 20 #467

Closes #466 deprecation 20 #467

bms63 commented Oct 8, 2024 •

edited

Loading

github-actions bot commented Oct 8, 2024 •

edited

Loading

bundfussr Oct 9, 2024

bms63 Oct 9, 2024

bundfussr Oct 9, 2024

bms63 Oct 9, 2024

ddsjoberg Oct 9, 2024

bundfussr Oct 9, 2024

bundfussr Oct 9, 2024

ddsjoberg Oct 9, 2024

bms63 Oct 9, 2024

bundfussr Oct 9, 2024

bms63 Oct 9, 2024

bundfussr Oct 9, 2024

ddsjoberg Oct 9, 2024

ddsjoberg Oct 9, 2024

ddsjoberg Oct 9, 2024

ddsjoberg Oct 9, 2024

ddsjoberg Oct 9, 2024

ddsjoberg left a comment

Closes #466 deprecation 20 #467

Are you sure you want to change the base?

Closes #466 deprecation 20 #467

Conversation

bms63 commented Oct 8, 2024 • edited Loading

github-actions bot commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddsjoberg left a comment

Choose a reason for hiding this comment

bms63 commented Oct 8, 2024 •

edited

Loading

github-actions bot commented Oct 8, 2024 •

edited

Loading