Closes #2441 refactor for modular design

[imccay13]

In response to https://github.com/pharmaverse/admiral/discussions/2349, refactors derive_vars_merged

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.

• [x] Place Closes # into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
• [x] Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
• [x] Updated relevant unit tests or have written new unit tests, which should consider realistic data scenarios and edge cases, e.g. empty datasets, errors, boundary cases etc. - See Unit Test Guide
• [x] If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
• [x] Review the Cheat Sheet. Make any required updates to it by editing the file inst/cheatsheet/admiral_cheatsheet.pptx and re-upload a PDF version of it to the same folder.
• [x] Update to all relevant roxygen headers and examples, including keywords and families. Refer to the categorization of functions to tag appropriate keyword/family.
• [x] Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• [x] Address any updates needed for vignettes and/or templates
• [x] Update NEWS.md under the header # admiral (development version) if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers). A Developer Notes section is available in NEWS.md for tracking developer-facing issues.
• [x] Build admiral site pkgdown::build_site() and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.
• [x] Address or fix all lintr warnings and errors - lintr::lint_package()
• [x] Run R CMD check locally and address all errors and warnings - devtools::check()
• [x] Link the issue in the Development Section on the right hand side.
• [x] Address all merge conflicts and resolve appropriately
• [x] Pat yourself on the back for a job well done! Much love to your accomplishment!

[bms63]

Hi @imccay13 I see this is draft, but just wanted to say there seems to be quite a few files that are not needed for this update in this PR.

[imccay13]

Hi @imccay13 I see this is draft, but just wanted to say there seems to be quite a few files that are not needed for this update in this PR.

@bms63 Believe I fixed this, and also fixed the problem I was having with the vignettes - let me know if I'm right about those two things and I'll open the PR

[github-actions[bot]]

Package	Line Rate	Health
admiral	97%	✔
Summary	97% (4721 / 4852)	✔

[bms63]

Hi @imccay13 if they pass all the checks (you can ignore link one), then please click ready for review. For the new function, I'm just glancing at this PR so far, I see only one unit test for it. Do you think that is sufficient coverage for the new function? I'd might error on the side of caution and have some dirty data scenarios and checks for messaging to users??

@bundfussr and @manciniedoardo can we prioritize a review as it might impact on admiralroche.

[manciniedoardo]

@millerg23 @bundfussr please could you give this a look-over with admiralroche in mind? thanks

[imccay13]

@bundfussr thank you for your comments, I implemented them all so far other than the ones describing what we're doing here: I think @paltusplintus will come up with something better than I can so I've looped him in - I'll re-request review when those documentation changes are made

[bms63]

ooohhh I was just having a thought about dirty data - should a simple message be returned to the user if nothing is flagged. I'm assuming the user would be expecting something if they are using this to derive something else...if nothing gets flagged then something might be amiss?? warning might be overkill

I guess they could also put in there own custom check...but if they go to the trouble to use this function we might want to give them something?

[bms63]

Hi @imccay13 - we have a release planned for June 2nd. It would be really nice to finish this PR by EOW please. Please implement our requested changes so we can re-review.

Also!! many thanks for contributing!! I know you work at GSK, but I'm not sure of your formal name.. :) Can we add you to the contributors to the Description file as well?

[bms63]

@imccay13 I created the Discussion as an issue and updated the title of the PR. This way once the PR merges into main it will automatically close the issue. The issue and discussion are linked together in case we need to chase back for whatever reason.

@paltusplintus and @imccay13 can this be completed by EOW with our requested changes? The code is fine, we just like our documentation and tests to be really clean and have pretty bows.

Release is happening on June 2nd.

[imccay13]

@imccay13 I created the Discussion as an issue and updated the title of the PR. This way once the PR merges into main it will automatically close the issue. The issue and discussion are linked together in case we need to chase back for whatever reason.

@paltusplintus and @imccay13 can this be completed by EOW with our requested changes? The code is fine, we just like our documentation and tests to be really clean and have pretty bows.

Release is happening on June 2nd.

@bms63 Yeah, that should be good - super feasible to have this done by EOW, and I've done all the low hanging fruit with docs, and I'll take a stab at the testing +try/catch this afternoon, as well as adding myself to contributors - will make a push to make sure this is all done EOW for sure

update: Just changed per the comments

[bms63]

@imccay13 can you make these last couple of tweaks and then we can merge in!! Also, please add yourself to description file or ping me in TEAMS and I can add you to the list