-
Notifications
You must be signed in to change notification settings - Fork 168
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
Remove duplicate definition #18
Conversation
Maybe we should just default to |
Changed default, because disconnection is practically always needed (and still will be for new-style DBI backends). |
R/src_dbi.R
Outdated
# stopifnot(is(con, "DBIConnection")) | ||
disco <- if (isTRUE(auto_disconnect)) db_disconnector(con) | ||
if (is_false(auto_disconnect)) |
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 use {}
here just to match tidyverse style?
Any idea why it's failing on R-devel? |
No idea. I purged the cache and re-triggered the build, need to wait for the result. |
All OK now. |
Thanks! Can you please add a bullet to NEWS? |
@@ -90,9 +90,13 @@ | |||
#' summarise(n = n()) %>% | |||
#' show_query() | |||
#' } | |||
src_dbi <- function(con, auto_disconnect = FALSE) { | |||
src_dbi <- function(con, auto_disconnect = TRUE) { |
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 can't be TRUE
because we create a bunch of transient src_dbi
s when working directly with DBI connections
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.
Or at least we need to change to this in dplyr:
#' @export
tbl.DBIConnection <- function(src, from, ...) {
check_dbplyr()
tbl(dbplyr::src_dbi(src), from = from, ..., auto_disconnect = FALSE)
}
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.
Sent PR to dplyr. A coarse CRAN search suggests that the default for src_dbi()
should be auto_disconnect = FALSE
, but only from the next dplyr release on.
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 rolled it back in dbplyr, I'm pretty sure defaulting to FALSE
is correct - you should set to TRUE
only when creating the connection in the src_dbi()
call, or when you know the scope of the source and the connection are identical.
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 meant to write the default should be auto_disconnect = TRUE
, but I don't mind either way. Thanks for reverting.
of
db_disconnector()
.Other (active) definition at
dbplyr/R/src_dbi.R
Line 119 in 2a4c041
How can we explicitly disconnect a source to avoid the message?