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

[actualben]

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():

• CatalogueExport/R/CatalogueExport.R

  Line 114 in 066b8e7

  if (missing(cdmVersion)) {

• CatalogueExport/R/CatalogueExport.R

  Line 142 in 066b8e7

  if (missing(sourceName) & !sqlOnly) {

• CatalogueExport/R/CatalogueExport.R

  Line 151 in 066b8e7

  if (!missing(analysisIds)) {

[actualben]

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]

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]

thanks I'll get a PR in this week