pharmaverse / admiral

ADaM in R Asset Library
https://pharmaverse.github.io/admiral
Apache License 2.0
220 stars 62 forks source link

Closes #1839 dummy function created #2350

Closed sheramin closed 6 months ago

sheramin commented 6 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 6 months ago

Code Coverage

Package Line Rate Health
admiral 98%
Summary 98% (4725 / 4814)
sheramin commented 6 months ago

@bms63 Thank you for reviewing it. I couldn't pass one of those checks (item h), which is running the pkgdown::build_site(). It seems there's a problem in one of the functions (please let me know if I need to post the function and file name). The CMD check passed too, but that one, nope! I'm attaching a snippet of that error: image

bms63 commented 6 months ago

@bms63 Thank you for reviewing it. I couldn't pass one of those checks (item h), which is running the pkgdown::build_site(). It seems there's a problem in one of the functions (please let me know if I need to post the function and file name). The CMD check passed too, but that one, nope! I'm attaching a snippet of that error: image

Interesting - what version of admiral do you have installed?

sheramin commented 6 months ago

@bms63 Thank you for reviewing it. I couldn't pass one of those checks (item h), which is running the pkgdown::build_site(). It seems there's a problem in one of the functions (please let me know if I need to post the function and file name). The CMD check passed too, but that one, nope! I'm attaching a snippet of that error: image

Interesting - what version of admiral do you have installed?

It's 1.0.1: image

bms63 commented 6 months ago

So that argument was deprecated, analysis_var, and I can see the function using set_values_to in the main branch

image

Can you try restarting the session and re-installing the package. Something fishy is going on!! It is something just not synching up correctly! :)

sheramin commented 6 months ago

So that argument was deprecated, analysis_var, and I can see the function using set_values_to in the main branch

image

Can you try restarting the session and re-installing the package. Something fishy is going on!! It is something just not synching up correctly! :)

I tried re-installing the package, duplicated the branch and restarting the session multiple times - I still get that error.

bms63 commented 6 months ago

@pharmaverse/admiral what are we missing?

@sheramin Can you verify that the code isn't there for analysis_var?

Also can you check your package panes to see if there are not multiple copies of admiral available? possible have it stored in another directory that pkgdown is pulling from??

sheramin commented 6 months ago

you verify that the code isn't there fo

@pharmaverse/admiral what are we missing?

@sheramin Can you verify that the code isn't there for analysis_var?

Also can you check your package panes to see if there are not multiple copies of admiral available? possible have it stored in another directory that pkgdown is pulling from??

Sure, I do not see analysis_var in the .Rmd file: image

But I see that variable in the .R file, although I'm not quite sure if that function gets called from derive_param_exposure.R image

It doesn't look like there are multiple copies of admiral. Just making sure I'm following where to look, here is a screenshot: image

manciniedoardo commented 6 months ago

@sheramin when you run devtools::document() does the .Rmd file update?

sheramin commented 6 months ago

@sheramin when you run devtools::document() does the .Rmd file update?

which .Rmd file? If you meant bds_exposure.Rmd, no it doesn't update that I believe (I checked the last modified date after running devtools::document. But, it does update some other files like NAMESPACE. I also updated the NEWS.md manually.

Here is what I get when run devtools::document() image

manciniedoardo commented 6 months ago

@sheramin my thinking was that running devtools::document() should regenerate the .Rmd documentation files, so for derive_param_exposure(), if your .R file was showing the newest version of the function then the .Rmd should update to show that too.

sheramin commented 6 months ago

@sheramin my thinking was that running devtools::document() should regenerate the .Rmd documentation files, so for derive_param_exposure(), if your .R file was showing the newest version of the function then the .Rmd should update to show that too.

@manciniedoardo thank you for following up. It looks like it doesn't regenerate the .Rmd files in vignettes directory (where bds_exposure.Rmd is located).

I think I'll try to remove the branch that I created from main, then recreate one and re-do the dummy issue, but I'm not sure if I'm authorized to deleted my own created branch.

sheramin commented 6 months ago

Thank you all! With Zelos (@zdz2101)'s help the issue resolved.

zdz2101 commented 6 months ago

Excellent job @sheramin , we will close, feel free to pick up issues as soon as you're comfortable to do so!