Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.envir parameter treated differently between glue() and glue_data() #308

Closed
ramiromagno opened this issue Jun 5, 2023 · 5 comments
Closed

Comments

@ramiromagno
Copy link

From the help page of both functions it seems .envir parameter ought to be treated in the same way. But glue::glue_data() seems to be picky and won't slurp lists.

glue::glue("{country} is the best!", .envir = list(country = "Portugal"))
#> Portugal is the best!
glue::glue_data("{country} is the best!", .envir = list(country = "Portugal"))
#> Error in new.env(hash = hash, parent = parent, size = size): 'enclos' must be an environment
@dave-lovell
Copy link

Interesting. glue() is just a wrapper around glue_data() that sets .x = NULL and passes all other arguments directly. So the difference in behaviour is actually caused by glue_data() accepting a list for .env when .x = NULL, but not when .x is missing:

## This crashes:
glue::glue_data("{country} is the best!", .envir = list(country = "Portugal"))
#> Error in new.env(hash = hash, parent = parent, size = size): 'enclos' must be an environment

## But this works:
glue::glue_data(.x = NULL, "{country} is the best!", .envir = list(country = "Portugal"))
#> Portugal is the best!

Created on 2023-09-15 with reprex v2.0.2

@dave-lovell
Copy link

dave-lovell commented Sep 15, 2023

Wait this is not the error you think it is - it's expected behavior!

... is the first argument of glue() and the second argument of glue_data(), after .x.

By not explicitly setting .x = NULL, you're implcitly doing:

glue_data(.x = "{country} is the best!", character(0),  .envir = list(country = "Portugal"))

The implied character(0) is because you've supplied nothing to ...

This eventually leads to glue_data() trying to do:

parent_env <- list2env(as.list("{country} is the best!"), parent = list(country = "Portugal"))
#> Error in new.env(hash = hash, parent = parent, size = size): 'enclos' must be an environment

Created on 2023-09-15 with reprex v2.0.2

@dave-lovell
Copy link

dave-lovell commented Sep 15, 2023

Okay but the behaviour is still inconsistent in the way that you've described:

library(dplyr)
library(glue)
library(rlang)

set.seed(123)

lil_iris <- slice_sample(iris, n = 3)

## You can specify .envir as a list when you use 'glue'
## (under the hood this is glue_data w/ .x = NULL)
glue("I love {country}!", .envir = list(country = "Portugal"))
#> I love Portugal!

## You can use an actual environment in conjunction with glue_data
glue_data(.x = lil_iris,
          "This {Species} has petal width {Petal.Width}. Also, I love {country}!",
          .envir = rlang::new_environment(data = list(country = "Portugal"))
)
#> This setosa has petal width 0.1. Also, I love Portugal!
#> This setosa has petal width 0.2. Also, I love Portugal!
#> This virginica has petal width 2.2. Also, I love Portugal!

## You can't use glue_data with .envir as a list
glue_data(.x = lil_iris,
          "This {Species} has petal width {Petal.Width}. Also, I love {country}!",
          .envir = list(country = "Portugal")
          )
#> Error in new.env(hash = hash, parent = parent, size = size): 'enclos' must be an environment

Created on 2023-09-15 with reprex v2.0.2

This is happening because when you specify .x and .envir in glue(), the evaluation environment for ... is made by converting the list .x into an environment, with the parent environment defined by .envir:

parent_env <- list2env(as.list(.x), parent = .envir)

A list cannot be a parent environment without first being converted into a list, so the function crashes.

If .x is NULL (this is always the case for glue(), the parent environment is set directly to .envir.

It's unclear to me whether accepting lists for .envir doesn't seem to be a deliberate feature. As far as I can see there are 3 options:

  1. Remove the ability use lists for .envir (this will probably break existing code)
  2. Make lists for .envir work in all cases, and document accordingly (this could be quite convenient, and glue is after all a convenience function)
  3. Ignore this, and let undocumented uses have unpredictable consequences

@jennybc
Copy link
Member

jennybc commented Sep 19, 2023

I think it's very uncommon to use .envir with glue_data(). In most organic usage, .x is the only argument a glue_data() user uses to make data available. This is borne out by inspecting the glue_data() calls found in CRAN packages.

If anything, the right solution is probably just to throw a better error message, i.e. to indicate that .envir is expected to be an actual environment in this case.

@jennybc
Copy link
Member

jennybc commented Sep 21, 2023

.envir is already documented to be an environment, so now we actually check for that. This brings behaviour more closely in line with the docs and the intended usage IMO.

jennybc added a commit that referenced this issue Dec 21, 2023
Unconditionally checking that .envir is an environment leads to breakage in revdeps. And I'm not convinced those revdeps are doing anything wrong.

This code suggests that glue_data() is designed to accept a non-environment as .envir, in some cases:

```
parent_env <- list2env(as.list(.x), parent = .envir)
```
jennybc added a commit that referenced this issue Jan 10, 2024
psychelzh added a commit to psychelzh/tarflow.iquizoo that referenced this issue Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants