Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support wrapping
englue()
#1566Support wrapping
englue()
#1566Changes from all commits
d408e28
d576a7c
11f8bab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
my_englue()
should also exposeenv
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would have the advantage of making it easy to wrap
names_glue
if the tidyr function adds englue support, without having to exposeenv
.Doesn't solve the error-call issue though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea being that the quosure created through the internal
enquo()
-like call would capture and pass on theenv
for you, right? Seems interesting.I guess that quosure env becomes the parent env if you want to insert a
.qux
pronoun or somethingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup we'd have to take
data
instead ofenv
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 useeval_tidy()
butglue()
in the end, I think that should still work).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong opinions, but do you have a reason for using
current_env()
overcaller_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 isenglue()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also mildly surprised this isn't
error_arg(x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases (tidyeval prog),
caller_arg(x)
will be a string instead of a symbol.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not great but it becomes too complex to document and use if we add a separate
error_call
for the user frame :-(Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.