-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add simple string interpolation for file from/to renaming #40
Conversation
c89f98a
to
02985c6
Compare
(pkgdown failure is unrelated to this PR, temporary failure with a certificate from the look of it) |
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.
Looks good to me, would be nice if query and files had the same syntax for referring to the same thing. One using r
from the environment as environment:r
the other as ${r}
which might be a bit confusing. But I understand why they are different and can't think of a better way to do that myself.
R/outpack_helpers.R
Outdated
##' @param envir An environment into which string interpolation may | ||
##' happen (see [orderly2::outpack_packet_use_dependency] for | ||
##' details on the string interpolation). The default here is to | ||
##' use the calling environment, whcih is typically reasonable, but |
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.
##' use the calling environment, whcih is typically reasonable, but | |
##' use the calling environment, which is typically reasonable, but |
R/outpack_packet.R
Outdated
##' In all cases, if you want to import a directory of files from a | ||
##' packet, you must refer to the source with a trailing slash | ||
##' (e.g., `c(here = "there/")`), which will create the local | ||
##' directory `here/...` with files from the upstream packet | ||
##' directory `there/`. If you omit the slash then an error will be | ||
##' thrown suggesting that you add a slash if this is what you | ||
##' intended. | ||
##' | ||
##' You can use a limited form of string interpolation in the names of | ||
##' this argument (or its `to` column if using a `data.frame`); | ||
##' using `${variable}` will pick up values from `envir` and | ||
##' substitute them into your string. This is similar to the | ||
##' interpolation you might be familiar with from `glue::glue` or | ||
##' similar, but much simpler with no concatenation or other fancy | ||
##' features supported. |
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 all sounds good
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.
ah, except for the vestigial comment about data.frames - I'll remove that
I've highlighted the inconsistency in the docs a bit |
This PR probably requires a bit more checking than most of the ones in the current batch.
The idea here is to work well in a loop we need to do something like:
The first part of this (the
environment:r
bit) is in #34, but the second part (the string interpolation on the third arg) is done here.An alternative would be something like:
which is (IMO) much uglier and error prone. I did wonder about adding data.frame support too, but removed it eventually to keep this PR simple (see 633c3e7).
I did think about a glue dependency for this bit of interpolation, but that implies we can do way too much, and so in the end I've gone for this bespoke solution. People can always use their own of course.
The docs would be easier to write if we rename
use
tofiles
inorderly_dependency