Support wrapping englue()
#1566
Conversation
|
Nice! |
| #' context by default). | ||
| #' | ||
| #' ```{r} | ||
| #' my_englue <- function(text) { |
There was a problem hiding this comment.
I think my_englue() should also expose env.
I think this is how we will end up documenting .envir in glue, like tidyverse/glue#281 (comment)
There was a problem hiding this comment.
I guess the main point is ensuring that caller_env() gets used and passed through somehow, so it isn't that important... but it still might be "best practice"?
Or maybe you can say:
If you are creating a low level package on top of
englue(), you should also consider exposingenv,error_arganderror_callin yourenglue()wrapper so users can wrap your wrapper.
There was a problem hiding this comment.
I like your note about exposing these arguments to allow wrappers.
There are cases where this would be unwieldy though, e.g. in the tidyr case.
By the way, maybe we should consider an enquo()-based interface for englue? We'd require literal strings as inputs and to wrap englue you'd use {{.
There was a problem hiding this comment.
This would have the advantage of making it easy to wrap names_glue if the tidyr function adds englue support, without having to expose env.
Doesn't solve the error-call issue though.
There was a problem hiding this comment.
The idea being that the quosure created through the internal enquo()-like call would capture and pass on the env for you, right? Seems interesting.
I guess that quosure env becomes the parent env if you want to insert a .qux pronoun or something
There was a problem hiding this comment.
yup we'd have to take data instead of env in this case. And if multiple layer masks are ever needed, we could extend to accept rlang data masks which make the layer structure explicit so we'd have all the information needed (even though we wouldn't use eval_tidy() but glue() in the end, I think that should still work).
There was a problem hiding this comment.
But the compiler would get us in trouble though, since it unwraps strings from promises and then we lose the environment. We can write this idea off.
| check_string(x) | ||
| englue <- function(x, | ||
| env = caller_env(), | ||
| error_call = current_env(), |
There was a problem hiding this comment.
I don't have strong opinions, but do you have a reason for using current_env() over caller_env()?
englue() will pretty much always be wrapped inside another function right? Should it default to reporting that function as the location of any issues?
There was a problem hiding this comment.
Most of the time englue() will be used as part of tidy eval to program against dplyr or tidyr. In these cases, I think it makes sense to match input errors to the function that the user used, which is englue().
| englue <- function(x, | ||
| env = caller_env(), | ||
| error_call = current_env(), | ||
| error_arg = "x") { |
There was a problem hiding this comment.
Also mildly surprised this isn't error_arg(x)
There was a problem hiding this comment.
In most cases (tidyeval prog), caller_arg(x) will be a string instead of a symbol.
| error_arg <- as_string(text_expr) | ||
| error_call <- NULL | ||
|
|
||
| # Find the relevant error frame. There are edge cases where this |
There was a problem hiding this comment.
This is the only part im a little iffy on but it seems reasonable and the downsides seem slim
There was a problem hiding this comment.
Not great but it becomes too complex to document and use if we add a separate error_call for the user frame :-(
Infer the user error frame from `env` instead of passing it around
Co-authored-by: Davis Vaughan <davis@rstudio.com>
8f3012b to
11f8bab
Compare
Closes #1565
cc @hadley