Closes #1728 merged_exist_msrc: Implement derive_flag_select_msrc()

[bundfussr]

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 devel branch until you have checked off each task.

• Place Closes #<insert_issue_number> into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
• Code is formatted according to the tidyverse style guide. Run styler::style_file() to style R and Rmd files
• 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
• If you removed/replaced any function and/or function parameters, did you fully follow the deprecation guidance?
• Update to all relevant roxygen headers and examples, including keywords and families. Refer to the categorization of functions to tag appropriate keyword/family.
• Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
• Address any updates needed for vignettes and/or templates
• 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)
• 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.
• Address or fix all lintr warnings and errors - lintr::lint_package()
• Run R CMD check locally and address all errors and warnings - devtools::check()
• Link the issue in the Development Section on the right hand side.
• Address all merge conflicts and resolve appropriately
• Pat yourself on the back for a job well done! Much love to your accomplishment!

[github-actions]

Package	Line Rate	Health
admiral	98%	✔
Summary	98% (4652 / 4740)	✔

[bms63]

I probably can't do a thorough review until Friday - hope that is okay!?!

[kaz462]

Not sure if this has been discussed - are we going to update the current naming convention? if we use derive_flag_select_msrc, should Programming strategy/function names be updated accordingly?

[bundfussr]

Not sure if this has been discussed - are we going to update the current naming convention? if we use derive_flag_select_msrc, should Programming strategy/function names be updated accordingly?

I have proposed a more consistent naming of the functions in https://phuseaccount.sharepoint.com/:w:/r/sites/Pharmaverse-admiral/Shared%20Documents/Development%20Team%20Working%20Docs/admiral%20Functions%20Overview.docx?d=w64c31636c93c47dda066585be7a8c42b&csf=1&web=1&e=NM6bIA. We decided not to implement it in 1.0 but most likely in 2.0.

At first I called the function derive_var_merged_exist_flag_msrc() but this was too long (lintr check failed). Thus I used the new name (in the hope that it will reduce the breaks of backward compatibility when we move to 2.0).

[kaz462]

derive_flag_select_msrc covers the functionality of derive_var_merged_exist_flag() - should we deprecate derive_var_merged_exist_flag?

[bms63]

This function is going to be very handy and is super clean!!! Always such a an awesome job Stefan!! I just had mostly cosmetic and layout comments.

For function name - can we call it derive_var_merged_ef_msrc() or something similar instead of derive_flag_select_mrsc() to keep consistent with our current naming scheme. This makes it easier to look up in the autocomplete in RStudio. I feel like if we go through with your re-naming request on so many functions then users are going to be frustrated anyways - having one function renamed to help with a possible later update doesn't make a lot of sense to me. The users are not going to like 2.0 :( Perhaps @manciniedoardo can make the final call on this functions name for us.

I must admit that I like the look of derive_flag_select_mrsc() by itself but when look at other merge functions it just seems out of place.

[manciniedoardo]

This function is going to be very handy and is super clean!!! Always such a an awesome job Stefan!! I just had mostly cosmetic and layout comments.

For function name - can we call it derive_var_merged_ef_msrc() or something similar instead of derive_flag_select_mrsc() to keep consistent with our current naming scheme. This makes it easier to look up in the autocomplete in RStudio. I feel like if we go through with your re-naming request on so many functions then users are going to be frustrated anyways - having one function renamed to help with a possible later update doesn't make a lot of sense to me. The users are not going to like 2.0 :( Perhaps @manciniedoardo can make the final call on this functions name for us.

I must admit that I like the look of derive_flag_select_mrsc() by itself but when look at other merge functions it just seems out of place.

I tend to agree. Our 1.0 release is underpinned by the premise of "maturity" i.e. users can commit to using this version long-term, and the package should be in such a state such that this is possible. One element of this is a consistent user experience, and as @bms63 noted, having a function whose name clashes with other functions just in light of a possible/future 2.0 update doesn't marry with this principle.

[bundfussr]

derive_flag_select_msrc covers the functionality of derive_var_merged_exist_flag() - should we deprecate derive_var_merged_exist_flag?

I think this requires discussion. I would not do it for admiral 1.0.0. Maybe you could add it to #2191.

[manciniedoardo]

derive_flag_select_msrc covers the functionality of derive_var_merged_exist_flag() - should we deprecate derive_var_merged_exist_flag?

I think this requires discussion. I would not do it for admiral 1.0.0. Maybe you could add it to #2191.

Agree, let's reduce impact to users for now.

[bundfussr]

This function is going to be very handy and is super clean!!! Always such a an awesome job Stefan!! I just had mostly cosmetic and layout comments.
For function name - can we call it derive_var_merged_ef_msrc() or something similar instead of derive_flag_select_mrsc() to keep consistent with our current naming scheme. This makes it easier to look up in the autocomplete in RStudio. I feel like if we go through with your re-naming request on so many functions then users are going to be frustrated anyways - having one function renamed to help with a possible later update doesn't make a lot of sense to me. The users are not going to like 2.0 :( Perhaps @manciniedoardo can make the final call on this functions name for us.
I must admit that I like the look of derive_flag_select_mrsc() by itself but when look at other merge functions it just seems out of place.

I tend to agree. Our 1.0 release is underpinned by the premise of "maturity" i.e. users can commit to using this version long-term, and the package should be in such a state such that this is possible. One element of this is a consistent user experience, and as @bms63 noted, having a function whose name clashes with other functions just in light of a possible/future 2.0 update doesn't marry with this principle.

Renamed as suggested

[bms63]

@kaz462 do you want to take a look at this or shall we merge in? @zdz2101 is OOO.

@bundfussr really awesome job. I'm looking forward to using this function!!

[kaz462]

LGTM! I will add discussion items to 2.0 Roadmap

derive_flag_select_msrc covers the functionality of derive_var_merged_exist_flag() - should we deprecate derive_var_merged_exist_flag?

I think this requires discussion. I would not do it for admiral 1.0.0. Maybe you could add it to #2191.

Agree, let's reduce impact to users for now.