-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
update str_extract to remove stringr dep #10
Conversation
Thanks @ayogasekaram !! We can leave the stringr standalone file as it is. But we do need to update lines 163 and 164 to not use standalone/R/standalone-check_pkg_installed.R Line 163 in 3a7ad70
|
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.
Lgtm! Thanks Abi :) Could you add a small test to the stringr standalone? Just to guarantee identical ouputs (I think they do generate identical outputs but it is good to have a very small check of this)
Hey @Melkiades Thank you for taking a look! As for the code, this is already in place in the stringr standalone script: https://github.com/insightsengineering/standalone/blob/3a7ad7048513a2b318d8171cef18eb75a6298315/R/standalone-stringr.R#L39C1-L45C2 and is already covered by the unit tests for this script. Is there a specific test I should add? |
I added some anyways :) Let me know if those suffice! |
For me it is great! So we are 100% sure that it does the same thing. I do not know if it is fine keeping the stringr in suggestion, though. If we keep it there, I would keep the test. What do you think @ddsjoberg? |
Awesome, thank you both!! Looks great! |
closes #9
Pre-review Checklist (if item does not apply, mark is as complete)
usethis::pr_merge_main()
last-updated
field has been updated.devtools::test_coverage()
Reviewer Checklist (if item does not apply, mark is as complete)
last-updated
field has been updated.pkgdown::build_site()
. Check the R console for errors, and review the rendered website.devtools::test_coverage()
When the branch is ready to be merged: