Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove duplicate definition #18

Merged
merged 6 commits into from
Oct 23, 2017
Merged

Remove duplicate definition #18

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b1e96b2
remove duplicate definition
krlmlr Jun 19, 2017
6748735
auto_disconnect is now TRUE by default and also supports NA
krlmlr Jun 19, 2017
5574f4b
add braces
krlmlr Jun 20, 2017
661d8ec
bump
krlmlr Jun 27, 2017
10dc27e
NEWS
krlmlr Jun 27, 2017
18d09f7
Merge branch 'master' into b-remove-dupe
hadley Oct 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions R/dbi-s3.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,21 +149,6 @@ random_table_name <- function(n = 10) {
paste0(sample(letters, n, replace = TRUE), collapse = "")
}

# Creates an environment that disconnects the database when it's
# garbage collected
db_disconnector <- function(con, name, quiet = FALSE) {
reg.finalizer(environment(), function(...) {
if (!quiet) {
message(
"Auto-disconnecting ", name, " connection ",
"(", paste(con@Id, collapse = ", "), ")"
)
}
dbDisconnect(con)
})
environment()
}

res_warn_incomplete <- function(res, hint = "n = -1") {
if (dbHasCompleted(res)) return()

Expand Down
11 changes: 7 additions & 4 deletions R/src_dbi.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
#' @param con An object that inherits from [DBI::DBIConnection-class],
#' typically generated by [DBI::dbConnect]
#' @param auto_disconnect Should the connection be automatically closed when
#' the src is deleted. This is useful for older DBI backends that don't
#' clean up themselves.
#' the src is deleted? Pass `NA` to auto-disconnect but print a message
#' when this happens.
#' @return An S3 object with class `src_dbi`, `src_sql`, `src`.
#' @export
#' @examples
Expand Down Expand Up @@ -90,9 +90,12 @@
#' summarise(n = n()) %>%
#' show_query()
#' }
src_dbi <- function(con, auto_disconnect = FALSE) {
src_dbi <- function(con, auto_disconnect = TRUE) {
Copy link
Member

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_dbis when working directly with DBI connections

Copy link
Member

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)
}

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

# stopifnot(is(con, "DBIConnection"))
disco <- if (isTRUE(auto_disconnect)) db_disconnector(con)
if (is_false(auto_disconnect))
Copy link
Member

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?

disco <- NULL
else
disco <- db_disconnector(con, quiet = is_true(auto_disconnect))

structure(
list(
Expand Down
6 changes: 3 additions & 3 deletions man/src_dbi.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.