-
Notifications
You must be signed in to change notification settings - Fork 13
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 #10 add function create_iso8601()
#21
Conversation
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 so much @ramiromagno, this is a great job 💠 💠 💠
So far I only had a chance to look at create_iso8601()
and run the Roche tests. I added my comments along the lines.
I also did a time check with 310K records and it was slightly faster than the function we have in Roche. I run both on the same machine:
# 310K records
# sdtm.oak::create_iso8601()
# user system elapsed
# 7.20 0.34 8.58
# roak::calc_iso_date_time()
# user system elapsed
# 7.51 0.79 9.22
To be fair, the above estimate was based on when I supplied all the known formats dtc_formats$fmt
. If I supply only one format then sdtm.oak
is the champion 🚀:
user system elapsed
2.43 0.18 2.76
I will continue reviewing the internal functions 👍
Co-authored-by: edgar-manukyan <[email protected]>
This function was likely added as part of an automatic setup of the R package as a whole but I guess we should add the `.onLoad()` if really needed.
Initially I thought of calling this function from within `assert_dtc_fmt()` but I think now that the current usage of `rlang::arg_match()` leads to more concise code, so this is preferred.
Import `.data` from rlang globally by using the R package level documentation (https://roxygen2.r-lib.org/articles/rd-other.html?q=_PACKAGE#packages).
…oak into 0010_calculate_isodate
* Add staged_dependencies for admiraldev * Add new line * Fix admiraldev links. * Fix admiraldev articles links. * Remove R 4.1 a it causing dependencies issues. We want to use purrr >= 1.0.0 * Test latest lintr * Test lintr with install package locally * Add install pacakge variable for lintr * Skip multi version pkgdown workflow. * R build ignore staged_dependencies.yaml
…oak into 0010_calculate_isodate
…oak into 0010_calculate_isodate
@ramiromagno, thanks so much for addressing Adam's and my comments 🙏 Everything looks awesome! I know that after the second approval you can merge, just wanted to make sure that at least one of our subject matter experts had a chance to try this awesomeness out. @rammprasad, will you have time? |
Yes, I am reviewing it today. Lets merge it tomorrow. |
Thank you so much for your feedback so far. I think we can still improve quite a bit the article about the |
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.
Great work, @ramiromagno . it is actually mindblowing. I have added some comments. Please take a look.
…ts and unk values
Hello @rammprasad: Just to be sure, I am only waiting for this pull request to be reviewed by you and then merged before I tackle these other two issues: #28 and #29. Is that okay? |
Thank you, @ramiromagno, for merging this to the main. We can work on other issues (maybe #18 ) before #28 and #29 |
Hi @rammprasad : I was not the one to perform the merging :) It was Adam. But I am happy to address those three issues :) |
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 oak 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.Request Title (Use Edit button in top-right if you need to update)
tidyverse style guide. Run
styler::style_file()
to style R and Rmd filesconsider realistic data scenarios and edge cases, e.g. empty datasets, errors,
boundary cases etc. - See
Unit Test Guide
devtools::document()
so all.Rd
files in theman
folder and theNAMESPACE
file in the project root are updated appropriatelyNEWS.md
if the changes pertain to a user-facing function (i.e. ithas an
@export
tag) or documentation aimed at users (rather than developers)pkgdown::build_site()
and check that all affectedexamples 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()
There is this NOTE: