rstudio / config

config package for R
https://rstudio.github.io/config/
256 stars 27 forks source link

expr blocks in unused configs are parsed and evaluated #20

Closed scottmmjackson closed 3 years ago

scottmmjackson commented 5 years ago

Reproduction steps:

  1. Create a new rmarkdown document or shiny application containing the following:
    library(config)
    c <- config::get()
    cat(paste(c$foo, config::is_active('irrelevant')))
  2. Create a new config.yml file in the content's folder:
    
    default:
    foo: bar

development: foo: development

irrelevant: foo: !expr BRRR::skrrrahh(sound = 26)


3. Run the content

#### Expected:

Standard output contains "bar", or "development" if `R_CONFIG_ACTIVE` is set to `development`, followed by FALSE.

#### Actual:

`Error in loadNamespace(name) : there is no package called ‘BRRR’`

-------

4. Now install https://github.com/brooke-watson/BRRR using `devtools::install_github('brooke-watson/BRRR')`

5. Run the content

#### Expected:

quiet

#### Actual:

Gucci Mane is played through the speakers irrespective of `R_CONFIG_ACTIVE`'s value.

"bar" or "development" is printed, followed by "FALSE", proving that `irrelevant` is not the config selected.
scottmmjackson commented 5 years ago

Related: https://github.com/rstudio/packrat/issues/510

scottmmjackson commented 5 years ago

It looks like this is caused by using eval.expr = TRUE in get:

https://github.com/rstudio/config/blob/master/R/get.R#L55

scottmmjackson commented 5 years ago

Reverting https://github.com/rstudio/config/pull/17 does not fix this problem.

konradsemsch commented 5 years ago

Hi guys, is anyone working on this issue? That's quite a blocker for deploying content to R Studio Connect by only using the keyring package. It would be very helpful to figure this one out!

blairj09 commented 4 years ago

This behavior comes from yaml::yaml.load_file(). If the expression is not immediately evaluated (using eval.expr = FALSE) then the resulting R list is:

$default
$default$foo
[1] "bar"

$development
$development$foo
[1] "development"

$irrelevant
$irrelevant$foo
[1] "BRRR::skrrrahh(sound = 26)"

The challenge here is that there's nothing indicating that the third entry in the list is actually an expression waiting for execution, which makes it difficult to defer execution to a later point.

slopp commented 4 years ago

@jimhester would you be the right person to ask about this? I know its a complicated matter since the yaml package is maintained separately. Alternatively, do you know who would be best equipped to show the config/keyring package some love?

andrie commented 3 years ago

I created a PR that I believe will fix the problem.

https://github.com/rstudio/config/pull/29

andrie commented 3 years ago

This should now be fixed in the master branch. I added test cases and I believe the new code base will handle this use case. Can you please test locally and report any problems?

cc @slopp , @blairj09

pmhogan commented 3 years ago

Thank you for a great package and the fix. Unfortunately updating to v0.3.1 appears to break magrittr pipes previously used within !expr. Reproduction steps:

config.yml

default:
    test: !expr 1 %>% add(1)

With v0.3:

library(magrittr)
> config::get(file = "config.yml")
$test
[1] 2

With v0.3.1:

library(magrittr)
> config::get(file = "config.yml")
Error in 1 %>% add(1) : could not find function "%>%"
andrie commented 3 years ago

@pmhogan , I'm sorry that this change broke your usage. As you discovered, the expressions are now handled in the base environment, so you will have to explicitly load external packages. Unfortunately this may cause you to duplicate some expressions, or to handle those expressions in your R script, rather than in the yaml expression.

Specifically, in your case:

default:
    test: !expr library(magrittr); 1 %>% add(1)
andrie commented 3 years ago

Closing this issue, since this behaviour was included in 8a939b2