Use of `missing()` makes catalogueExport a little harder to use

actualben commented 2 years ago

Currently the catalogueExport function uses the built-in R function missing() to check for non-default function arguments:

 if (!missing(analysisIds)) {

This is in contrast to checking the argument's value:

 if (analysisIds != "") {

This makes life a bit harder for code that calls catalogueExport because you can't do something like this:

config <- loadConfig()
catalogueExport(
    config$connectionDetails,
    cdmDatabaseSchema = config$cdmSchema,
    analysisIds = config$analysisIds
)

...because no possible value of config$analysisIds can specify that you want the default behavior (where no analysisIds argument was specified).

I've tried using substitute() to make an argument appear to be missing, but then that masks the default value of analysisIds from the catalogueExport function definition.

I can use conditionals to build up a list of function arguments and pass that via do.call but that makes the code a bit less clear and it fixes my problem but leaves it for other people.

How would you feel about a PR that replaces missing() checks with value checks?

The current uses of missing():

actualben commented 2 years ago

BTW here's an article/video from a prominent R developer (former Rstudio employee, current senior engineer at Netflix, former Bioconductor core dev) which recommends not using missing() for this purpose https://www.jimhester.com/post/2019-04-17-missing/

MaximMoinat commented 2 years ago

Good point. If I can summarise: the default values in the catalogueExport are never used. If not user-provided, they will be overridden by whatever is in the if-body. -edit- upon rereading; another issue is that there is no way to provide an argument (from another part of the code) that triggers the default behaviour.

If you can open a PR, that would be great. Thanks already for all the detailed info!

actualben commented 2 years ago

thanks I'll get a PR in this week

EHDEN / CatalogueExport

Use of `missing()` makes catalogueExport a little harder to use #51