-
Notifications
You must be signed in to change notification settings - Fork 190
Add params= to dbExecute S3 method
#667
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
base: main
Are you sure you want to change the base?
Conversation
params= to dbExecute S3 methodparams= to dbExecute S3 method
R/dbi-connection.R
Outdated
| quiet = conn@quiet, | ||
| ... | ||
| dots <- list(...) | ||
| if (all(c("params", "parameters") %in% names(dots))) { |
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 this would be cleaner if you had explicit params and parameters arguments and then used check_exclusive(params, parameters). (Then I think you can eliminate the do.call below)
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 can do that. Overall, the options:
- Change
bq_*(..., parameters=)tobq_*(..., params=). This was the "best" in my mind because it requires nocheck_exclusive()or other fancy checking, it "just worked naturally" with the ellipses. The reason I did not go with this was that I assumed that the overall package (or tidy) preference was towardsparameters=for other reasons, regardless of the precedent set byDBI. - What I did (which was a workaround for not knowing about
rlang::check_exclusive()).
I'll work on (2) unless you (quickly) come back with permission to implement (1) above.
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 I need something other than check_exclusive(params, parameters).
With the dbSendQuery S3 method, I can't add parameters= in order to immediately check_exclusive(), yet when it calls BigQueryResult(params=params), the params= is no longer missing. I think I need to check for "parameters" %in% names(list(...)) to preclude this. Is there another way to pass-along missingness in a call like that?
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.
Can you make a helper? This works for me:
f <- function(...) {
check_params(...)
list(...)
}
check_params <- function(param, parameters, ...) {
check_exclusive(param, parameters, .require = FALSE)
}
f(param = 1)
f(parameters = 1)
f(parameters = 1, param = 2)(But I may have forgotten a wrinkle that makes this challenging)
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 really.
The issue is that dbGetQuery calls dbSendQuery which hard-codes params = params, so downstream functions see it as NULL instead of missing. I cannot add parameters= to the S3 method arguments without triggering an issue about a different S3 signature.
Instead, I've copied check_exclusive into a local function check_exclusive_missing_or_null that extends rlang::check_exclusive to add && !is.null(!!.x) inside the inject. (I changed a couple of other functions that are not being imported, no problem.)
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.
Two notes:
- I've worked around not having them, but since
check_exclusiveuseseveryandformat_arg, it might be useful to useful to either addcheck_exclusive(..., .null=FALSE)or definecheck_exclusive_missing_or_nullwithinrlang. I'm assuming neither for now. bigrquerydoes not importpurrrdirectly but it is imported viarlang, somap_*is not immediately available. Would you like me to add@importFrom purrr ...to the imports, or are you fine with me shifting tovapply?
BTW, the function above resolves my intermediate issue, I'll push to the PR once I hear back from you on this.
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.
Let me just take a swing at it myself.
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.
FTR,
check_exclusive_missing_or_null <- function(..., .require = TRUE, .frame = caller_env(), .call = .frame) {
args <- enexprs(..., .named = TRUE)
if (length(args) < 2) {
cli::cli_abort("Must supply at least two arguments.")
}
if (!all(sapply(args, is.symbol))) {
cli::cli_abort("`...` must be function arguments.")
}
present <- vapply(args, function(.x) inject(!base::missing(!!.x) && !is.null(!!.x), .frame), logical(1))
n_present <- sum(present)
if (n_present == 0) {
if (.require) {
args <- names(args)
names(args) <- args
args[] <- paste0("`", args, "`")
enum <- oxford_comma(args)
msg <- sprintf("One of %s must be supplied.", enum)
cli::cli_abort(msg, call = .call)
}
else {
return("")
}
}
if (n_present == 1) {
return(as_string(args[[which(present)]]))
}
args <- names(args)
names(args) <- args
args[] <- paste0("`", args, "`")
enum <- oxford_comma(args)
msg <- sprintf("Exactly one of %s must be supplied.", enum)
if (n_present != length(args)) {
enum <- oxford_comma(args[present], final = "and")
msg <- c(msg, x = sprintf("%s were supplied together.",
enum))
}
cli::cli_abort(msg, call = .call)
}
Two fixes:
dbGetQuery(con, query, params=)works,dbExecute(con, query, params=)does not; andparams=andparameters=from being passed asBigQueryResult(params=)Includes NEWS, code, and unit-tests.