-
Notifications
You must be signed in to change notification settings - Fork 64
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 #2336 fix bug in derive_param_tte #2353
Conversation
NEWS.md
Outdated
@@ -1,3 +1,7 @@ | |||
# admiral 1.0.2 | |||
|
|||
- Fix bug in `derive_param_tte()` where argument `dataset` populated and argument `by_vars` is dynamic. (#2336) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the issue was due to duplicated information being detected?
@@ -805,33 +805,135 @@ test_that("derive_param_tte Test 11: ensuring ADT is not NA because of missing s | |||
) | |||
}) | |||
|
|||
## Test 12: test dataset and dynamic byvars populated ---- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by what dynamic means here? sorry!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, its when the PARAMCD in SET_VALUES_TO is not static, not BY_VARS
. I will re-word. See new unit test, the PARAMCD is an expression, not a constant value.
Co-authored-by: Stefan Bundfuss <[email protected]>
NEWS.md
Outdated
@@ -1,3 +1,7 @@ | |||
# admiral 1.0.2 | |||
|
|||
- Fix bug in `derive_param_tte()` where argument `dataset` populated and `PARAMCD` in `set_values_to` argument is an expression. (#2336) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Gordon - I guess I am still confused about what we are telling the user here, what we are fixing and there might be a dose of me not fully following :)
The bug is from the function picking up potential false positives around the parameters and falling over with the duplication error?
The fix is now we check to make sure the parameters do not already exist in the datasets and give feedback to the user?
I feel like reading the issue and news bullet has only confused me!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the check was looking at the contents of what PARAMCD
was in set_values_to
argument, and seeing if the PARAMCD
already existed in dataset that is passed into dataset
argument. If PARAMCD
was not an expression ie PARAMCD = "XYZ"
then check worked. However, check was very early in function, so if PARAMCD
to be created was an expression, and wasn't resolved yet, this caused the ERROR. I moved the check to near the end, where PARAMCD
is resolved in the dataset holding the new params. Happy to change the wording in the NEWS item!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooohhh could that be the new summary just a little more polished please.
thanks again for digging into this and ELI5 to me!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, any further updates via suggestions
are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @millerg23!
@bundfussr @rossfarrugia @manciniedoardo
Is it okay to deliver to CRAN on March 4th. That way we have a full month between releases?
* Closes #2311 attend to `derive_vars_query()` bug (#2313) * new branch here * fix typo in news * 2311 updated unit test to have mixed case * missing the toupper part * add more detail to news * fix links * add documentation on case insensitivity --------- Co-authored-by: Gordon Miller <[email protected]> * chore: #2311 remove renv to get website to build * [actions skip] Add/Update README.md for patch * Closes #2336 fix bug in derive_param_tte (#2353) * 2336 fix bug * 2236 fix STYLER and LINTR * 2236 fix (reword) NEWS item * Update NEWS.md Co-authored-by: Stefan Bundfuss <[email protected]> * 2336 select unique PARAMCD before check * 2336 update NEWS item to be more informative * 2336 fix typo --------- Co-authored-by: Stefan Bundfuss <[email protected]> * chore: remove readme render in actions --------- Co-authored-by: Zelos Zhu <[email protected]> Co-authored-by: Gordon Miller <[email protected]> Co-authored-by: GitHub Actions <[email protected]> Co-authored-by: Gordon Miller <[email protected]> Co-authored-by: Stefan Bundfuss <[email protected]>
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.styler::style_file()
to style R and Rmd filesinst/cheatsheet/admiral_cheatsheet.pptx
and re-upload a PDF version of it to the same folder.devtools::document()
so all.Rd
files in theman
folder and theNAMESPACE
file in the project root are updated appropriatelyNEWS.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)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 check
locally and address all errors and warnings -devtools::check()