253 feature request deprecatesupersede derive vars crit #254
Conversation
federicobaratin
added
documentation
ahasoplakus
requested review from
ahasoplakus and
manciniedoardo
and removed request for
vikrams95
|
|
Hi @federicobaratin I made some changes to yours to align with the phase 1 deprecation strategy of |
| @@ -1,16 +1,5 @@ | |||
| # This is a list of urls to ignore in the link checking action Check URLS / | |||
There was a problem hiding this comment.
Is it intended to remove all this stuff here?
| @@ -2,11 +2,9 @@ | |||
| #' | |||
| #' | |||
| #' @description | |||
| #' Derive analysis criterion evaluation result variable, paired with character | |||
There was a problem hiding this comment.
why delete this section? i think ok to keep, just need to add the badge.
| #' | ||
| derive_vars_crit <- function(dataset, prefix, crit_label, condition, criterion) { | ||
| condition <- assert_filter_cond(enquo(condition)) |
There was a problem hiding this comment.
This is not in line with our deprecation strategy.
Here you have deleted all the function code and just added a warning. So the function behaves completely differently to before, as it is essentially just a warning thrower. If I am using this function in any code and upversion admiralvaccine, now my code will error or at least not derive the variables. Instead, what we want is for the code to still work, but a warning to be thrown (at least for this 1st Phase of deprecation, see again here
for details).
There was a problem hiding this comment.
Hi Edoardo, I can take it. Just one question: on the Phase 1 strategy is suggested to put a warning + replace the "old" function code with the new one. Is this also the case? Or, for the moment, a simple warning + "old" function code is needed?
There was a problem hiding this comment.
as you are implementing phase 1, you should just add the warning at the top of the function and then keep all else the same. you should also not delete documentation.
There was a problem hiding this comment.
just to add, in your case you don't have any actual new code to replace as you are just deprecating this function in favour of one from another package (admiral)
There was a problem hiding this comment.
This is not in line with our deprecation strategy.
Here you have deleted all the function code and just added a warning. So the function behaves completely differently to before, as it is essentially just a warning thrower. If I am using this function in any code and upversion admiralvaccine, now my code will error or at least not derive the variables. Instead, what we want is for the code to still work, but a warning to be thrown (at least for this 1st Phase of deprecation, see again here for details).
Hi @manciniedoardo Thanks for the review! We have removed the old code and replaced it with a deprecate_warn (maybe it should be replaced with deprecate_inform) and the new admiral::derive_vars_crit_flag. So, I guess the code will still work or am I missing something here?
There was a problem hiding this comment.
Ok, thanks! I have just updated the documentation as per suggestions. @ahasoplakus , I kept the warning you put at the top of the function.
There was a problem hiding this comment.
@ahasoplakus . I just seen that you are working on that. I made a couple of simple updates, so I would stop here just to avoid confusion. Please, let me know if any change is required from my side
There was a problem hiding this comment.
@ahasoplakus . I just seen that you are working on that. I made a couple of simple updates, so I would stop here just to avoid confusion. Please, let me know if any change is required from my side
Thanks @federicobaratin Please feel free to go ahead with the changes @manciniedoardo suggested and fix the checks that are currently failing. I will do a final review and merge
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
develbranch until you have checked off each task.styler::style_file()to style R and Rmd filesdevtools::document()so all.Rdfiles in themanfolder and theNAMESPACEfile in the project root are updated appropriatelyNEWS.mdif the changes pertain to a user-facing function (i.e. it has an@exporttag) or documentation aimed at users (rather than developers)pkgdown::build_site()and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.lintr::lint_package()R CMD checklocally and address all errors and warnings -devtools::check()