search

pharmaverse / admiral.test

Test SDTM data for use with admiral
Other
2 stars 1 forks source link

Move updating scripts to dev folder #62

Closed bms63 closed 2 years ago

bms63 commented 2 years ago

To keep package lean and mean and just focused on data, we have decided to move the data scripts that alter and update the CDSIC pilot data to a dev folder.

DoD: dev folder has been created and scripts moved and folder registered in .Rbuildignore

Concerns:

thomas-neitmann commented 2 years ago

Make sure that this dev folder is listed in .Rbuildignore such that it won't be included when building the package and shipping to CRAN.

bms63 commented 2 years ago

I see we are just doing .Rbuildignore with inst/templates . Shall we consider this done or do we want to make the dev folder?

bundfussr commented 2 years ago

@thomas-neitmann , I think moving get_terms.R to the dev folders causes failure when building the vignettes in admiral (see https://github.com/pharmaverse/admiral/runs/7217762791?check_suite_focus=true). Could we revert it?

thomas-neitmann commented 2 years ago

When we move the functions back into the R folder we'll have to re-list the dependency packages in the DESCRIPTION file. However, we wanted {admiral.test} to be dependency free. Thus, my suggestion would be to source() the function directly from GitHub in the affected {admiral} examples.

bundfussr commented 2 years ago

How do we control the branch from which the functions are sourced? Usually we use the devel version when merging to devel and the main version when merging to main. But the code in the examples is fixed.

bms63 commented 2 years ago

Can we move to get_terms to admiral or rebuild with base R?

bundfussr commented 2 years ago

We added get_terms to admiral.test to avoid that these functions are available in admiral and may be used by accident. Users will use admiral but not admiral.test in their scripts.

bundfussr commented 2 years ago

By the way, why are the dependencies in admiral.test a problem?

bms63 commented 2 years ago

i think we are just trying to make admiral.test a data package. The updating of the test data means we have more scripts and need more packages to make those updates. Moving all of them to a dev folder cuts out those dependencies.

However, we may need to rethink it a a bit. @thomas-neitmann

thomas-neitmann commented 2 years ago

We could indeed rewrite the function to only use base R functionality. From my quck glance at the function that shouldn't be too difficult.

However, since this is really only an example function it doesn't really feel like a correct approach to have it as an exported function in {admiral.test}.

That leaves moving the function over to {admiral} itself. We could put it directly in the example section for derive_vars_query() with a comments above that this is an example function and that user would have to define somethig similar for themselves.

bundfussr commented 2 years ago

Could we hide the definition of the function in the example? Could \dontshow work?

Showing the code of the function in the example could be confusing because it is a dummy function. The code would be completely different in a real application,

bundfussr commented 2 years ago

I tried using \dontshow. The code is not displayed when using the help in Rstudio but it is displayed on the webpage, which does not look nice. I also noticed that the functions use admiral_smq_db and admiral_sdg_db. I think we should keep the data and the function together.

I would prefer to keep the functions in admiral.test. It seems to be the simplest solution. As admiral.test provides example data, it seems reasonable to me that it provides also example functions. (By the way, the function are not exported and called in the examples via admiral.test:::. So they can not be used by accident even if admiral.test is loaded and it is obvious in the examples that this part need to be updated by the user.)

thomas-neitmann commented 2 years ago

Alright, then let's keep the function in {admiral.test}. I'll make some small updates to get rid of the {dplyr} functions used in the function such that we can stick to the no-dependency strategy for {admiral.test}.

bundfussr commented 2 years ago

@thomas-neitmann , could you fix it? The issue is preventing us from merging any PRs in admiral.

thomas-neitmann commented 2 years ago

Working on it right now.