pharmaverse / admiralpeds

Admiral Package Extension for Pediatric Clinical Trials
https://pharmaverse.github.io/admiralpeds/
Apache License 2.0
13 stars 3 forks source link

Closes #62 advs template add interpolation functions #68

Closed Fanny-Gautier closed 4 months ago

Fanny-Gautier commented 4 months ago

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.

github-actions[bot] commented 4 months ago

Code Coverage

Package Line Rate Health
admiralpeds 83%
Summary 83% (159 / 192)
Fanny-Gautier commented 4 months ago

I am currently unable to run pkgdown::build_site() with the default value NULL defined in derive_interp_records() image

Fanny-Gautier commented 4 months ago

I am currently unable to run pkgdown::build_site() with the default value NULL defined in derive_interp_records() image

I finally could run pkgdown::build_site()but do I need to call library(admiral) in addition to dplyr for the below error? image

rossfarrugia commented 4 months ago

I finally could run pkgdown::build_site()but do I need to call library(admiral) in addition to dplyr for the below error?

sure, add library(admiral, warn.conflicts = FALSE) and see if that resolves

Fanny-Gautier commented 4 months ago

@zdz2101 , @rossfarrugia : I am still facing the error with by_vars since using assert_vars() in the example. Could you please suggest any solution ? I have unsuccessfully worked around the whole afternoon. image image Thanks.

All other comments are implemented. Please let me know if there are further comments.

rossfarrugia commented 4 months ago

@zdz2101 , @rossfarrugia : I am still facing the error with by_vars since using assert_vars() in the example. Could you please suggest any solution ? I have unsuccessfully worked around the whole afternoon.

@bundfussr can you take a look at this PR please and the error when we run pkgdown::build_site(). It's something to do with the assertions as by_vars is allowed to be NULL. Appreciate you sanity checking as you'd probably find the issue a million times quicker than me.

bundfussr commented 4 months ago

@zdz2101 , @rossfarrugia : I am still facing the error with by_vars since using assert_vars() in the example. Could you please suggest any solution ? I have unsuccessfully worked around the whole afternoon.

@bundfussr can you take a look at this PR please and the error when we run pkgdown::build_site(). It's something to do with the assertions as by_vars is allowed to be NULL. Appreciate you sanity checking as you'd probably find the issue a million times quicker than me.

I'll have a look.

bundfussr commented 4 months ago

@zdz2101 , @rossfarrugia : I am still facing the error with by_vars since using assert_vars() in the example. Could you please suggest any solution ? I have unsuccessfully worked around the whole afternoon.

@bundfussr can you take a look at this PR please and the error when we run pkgdown::build_site(). It's something to do with the assertions as by_vars is allowed to be NULL. Appreciate you sanity checking as you'd probably find the issue a million times quicker than me.

Adding library(rlang) at the beginning should solve the error when we run pkgdown::build_site().

rossfarrugia commented 4 months ago

thanks @bundfussr! @Fanny-Gautier please add as library(rlang, warn.conflicts = FALSE) for consistency with Zelos's functions. Even though I don't think there are any conflicts anyway...

bundfussr commented 4 months ago

By the way, superseded dplyr functions like group_by_at() and do() are used in the code. I wonder if this should be avoided.

bundfussr commented 4 months ago

And to be in line with the admiral messaging cli functions should be used for messages, e.g., cli_abort() instead of stop().

rossfarrugia commented 4 months ago

@Fanny-Gautier appreciate if you can look into Stefan's 2 comments above and undo my bad suggestions above - sorry 😞 and then hopefully we can get this one merged!

Fanny-Gautier commented 4 months ago

Could you please let me know how to handle the below "error" ? When running styler, it set back my lines to 6 spaces, while Linter requires 21 spaces. So when it fits to Linter this makes a styler error, and when it fits to styler, it makes a Linter error. image

bundfussr commented 4 months ago

Could you please let me know how to handle the below "error" ? When running styler, it set back my lines to 6 spaces, while Linter requires 21 spaces. So when it fits to Linter this makes a styler error, and when it fits to styler, it makes a Linter error. image

Yes, this is a known issue. Please add indentation_linter=NULL, to the .lintr file. This should solve the issue.

rossfarrugia commented 4 months ago

@Fanny-Gautier i ran through the ADVS template and the results look good. i also checked test coverage using covr and your new function is 100% covered. so i'll approve/merge this now so that i can merge into my open PR.

@bundfussr if you spot any further code improvements for this one please can you make an issue for us and we can tackle separately to this PR.