Merge pull request #536 from cynkra/f-check-dots-empty

- `*_pk()` and `*_fk()` functions now verify that the dots are actually empty (#536).

R/foreign-keys.R

@@ -46,6 +46,7 @@
 #'   dm_add_fk(flights, tailnum, planes) %>%
 #'   dm_draw()
 dm_add_fk <- function(dm, table, columns, ref_table, ..., check = FALSE) {
+  check_dots_empty()
   check_not_zoomed(dm)
   table_name <- dm_tbl_name(dm, {{ table }})
   ref_table_name <- dm_tbl_name(dm, {{ ref_table }})
@@ -120,6 +121,7 @@ dm_add_fk_impl <- function(dm, table, column, ref_table) {
 #' dm_nycflights13() %>%
 #'   dm_has_fk(airports, flights)
 dm_has_fk <- function(dm, table, ref_table, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
   table_name <- dm_tbl_name(dm, {{ table }})
   ref_table_name <- dm_tbl_name(dm, {{ ref_table }})
@@ -157,6 +159,7 @@ dm_has_fk_impl <- function(dm, table_name, ref_table_name) {
 #' dm_nycflights13(cycle = TRUE) %>%
 #'   dm_get_fk(flights, airports)
 dm_get_fk <- function(dm, table, ref_table, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
 
   table_name <- dm_tbl_name(dm, {{ table }})
@@ -201,6 +204,7 @@ dm_get_fk_impl <- function(dm, table_name, ref_table_name) {
 #'   dm_get_all_fks()
 #' @export
 dm_get_all_fks <- function(dm, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
   dm_get_all_fks_impl(dm)
 }
@@ -237,6 +241,7 @@ dm_get_all_fks_impl <- function(dm) {
 #'   dm_rm_fk(flights, dest, airports) %>%
 #'   dm_draw()
 dm_rm_fk <- function(dm, table, columns, ref_table, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
 
   column_quo <- enquo(columns)
@@ -329,6 +334,7 @@ dm_rm_fk_impl <- function(dm, table_name, cols, ref_table_name) {
 #'   enum_fk_candidates(airports)
 #' @export
 dm_enum_fk_candidates <- function(dm, table, ref_table, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
   # FIXME: with "direct" filter maybe no check necessary: but do we want to check
   # for tables retrieved with `tbl()` or with `dm_get_tables()[[table_name]]`
@@ -352,6 +358,7 @@ dm_enum_fk_candidates <- function(dm, table, ref_table, ...) {
 #' @param zoomed_dm A `dm` with a zoomed table.
 #' @export
 enum_fk_candidates <- function(zoomed_dm, ref_table, ...) {
+  check_dots_empty()
   check_zoomed(zoomed_dm)
   check_no_filter(zoomed_dm)
 

R/primary-keys.R

@@ -51,6 +51,7 @@
 #'     dm_add_pk(planes, manufacturer, check = TRUE)
 #' )
 dm_add_pk <- function(dm, table, columns, ..., check = FALSE, force = FALSE) {
+  check_dots_empty()
   check_not_zoomed(dm)
   table_name <- dm_tbl_name(dm, {{ table }})
 
@@ -104,6 +105,7 @@ dm_add_pk_impl <- function(dm, table, column, force) {
 #'   dm_has_pk(planes)
 #' @export
 dm_has_pk <- function(dm, table, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
   table_name <- dm_tbl_name(dm, {{ table }})
   dm_has_pk_impl(dm, table_name)
@@ -145,6 +147,7 @@ dm_has_pk_impl <- function(dm, table) {
 #'   dm_get_pk(planes)
 #' @export
 dm_get_pk <- function(dm, table, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
   table_name <- dm_tbl_name(dm, {{ table }})
   new_keys(dm_get_pk_impl(dm, table_name))
@@ -183,6 +186,7 @@ dm_get_pk_impl <- function(dm, table_name) {
 #' dm_nycflights13() %>%
 #'   dm_get_all_pks()
 dm_get_all_pks <- function(dm, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
   dm_get_all_pks_impl(dm)
 }
@@ -219,6 +223,7 @@ dm_get_all_pks_def_impl <- function(def) {
 #'   dm_draw()
 #' @export
 dm_rm_pk <- function(dm, table, ..., rm_referencing_fks = FALSE) {
+  check_dots_empty()
   check_not_zoomed(dm)
   table_name <- dm_tbl_name(dm, {{ table }})
 
@@ -273,6 +278,7 @@ dm_rm_pk_impl <- function(dm, table_name) {
 #' nycflights13::flights %>%
 #'   enum_pk_candidates()
 enum_pk_candidates <- function(table, ...) {
+  check_dots_empty()
   # a list of ayes and noes:
   if (is_dm(table) && is_zoomed(table)) table <- get_zoomed_tbl(table)
 
@@ -295,6 +301,7 @@ enum_pk_candidates <- function(table, ...) {
 #' dm_nycflights13() %>%
 #'   dm_enum_pk_candidates(airports)
 dm_enum_pk_candidates <- function(dm, table, ...) {
+  check_dots_empty()
   check_not_zoomed(dm)
   # FIXME: with "direct" filter maybe no check necessary: but do we want to check
   # for tables retrieved with `tbl()` or with `dm_get_tables()[[table_name]]`

R/zzx-deprecated.R

@@ -272,15 +272,15 @@ cdm_apply_filters_to_tbl <- function(dm, table) {
 #' @export
 cdm_add_pk <- function(dm, table, column, check = FALSE, force = FALSE) {
   deprecate_soft("0.1.0", "dm::cdm_add_pk()", "dm::dm_add_pk()")
-  dm_add_pk(dm, {{ table }}, {{ column }}, check, force)
+  dm_add_pk(dm, {{ table }}, {{ column }}, check = check, force = force)
 }
 
 #' @rdname deprecated
 #' @keywords internal
 #' @export
 cdm_add_fk <- function(dm, table, column, ref_table, check = FALSE) {
   deprecate_soft("0.1.0", "dm::cdm_add_fk()", "dm::dm_add_fk()")
-  dm_add_fk(dm, {{ table }}, {{ column }}, {{ ref_table }}, check)
+  dm_add_fk(dm, {{ table }}, {{ column }}, {{ ref_table }}, check = check)
 }
 
 #' @rdname deprecated

tests/testthat/_snaps/rows-dm.md

@@ -61,6 +61,8 @@
     Output
       airlines airports  flights   planes  weather 
             16     1458     1761     3322      144 
+    Code
+      dbDisconnect(sqlite)
 
 # dm_rows_update()
 

tests/testthat/test-dm.R

@@ -1,13 +1,3 @@
-test_that("can access tables", {
-  skip_if_not_installed("nycflights13")
-
-  expect_identical(tbl(dm_nycflights13(), "airlines"), nycflights13::airlines)
-  expect_dm_error(
-    tbl_impl(dm_nycflights13(), "x"),
-    class = "table_not_in_dm"
-  )
-})
-
 test_that("can create dm with as_dm()", {
   expect_equivalent_dm(as_dm(dm_get_tables(dm_test_obj())), dm_test_obj())
 })
@@ -22,85 +12,6 @@ test_that("creation of empty `dm` works", {
   )
 })
 
-test_that("'copy_to.dm()' works", {
-  expect_dm_error(
-    copy_to(dm_for_filter(), letters[1:5], name = "letters"),
-    "only_data_frames_supported"
-  )
-
-  expect_dm_error(
-    copy_to(dm_for_filter(), list(mtcars, iris)),
-    "only_data_frames_supported"
-  )
-
-  expect_dm_error(
-    copy_to(dm_for_filter(), mtcars, overwrite = TRUE),
-    "no_overwrite"
-  )
-
-  skip_if_src_not("df", "mssql")
-
-  # `tibble()` call necessary, #322
-  car_table <- test_src_frame(!!!mtcars)
-
-  expect_equivalent_dm(
-    suppress_mssql_message(copy_to(dm_for_filter(), mtcars, "car_table")),
-    dm_add_tbl(dm_for_filter(), car_table)
-  )
-
-  # FIXME: Why do we do name repair in copy_to()?
-  expect_equivalent_dm(
-    suppress_mssql_message(expect_name_repair_message(
-      copy_to(dm_for_filter(), mtcars, "")
-    )),
-    dm_add_tbl(dm_for_filter(), ...7 = car_table)
-  )
-})
-
-test_that("'copy_to.dm()' works (2)", {
-  local_options(lifecycle_verbosity = "quiet")
-
-  expect_dm_error(
-    copy_to(dm(), mtcars, c("car_table", "another_table")),
-    "one_name_for_copy_to"
-  )
-
-  # rename old and new tables if `repair = unique`
-  expect_name_repair_message(
-    expect_equivalent_dm(
-      copy_to(dm(mtcars), mtcars),
-      dm(mtcars...1 = mtcars, mtcars...2 = tibble(mtcars))
-    )
-  )
-
-  expect_equivalent_dm(
-    expect_silent(
-      copy_to(dm(mtcars), mtcars, quiet = TRUE)
-    ),
-    dm(mtcars...1 = mtcars, mtcars...2 = tibble(mtcars))
-  )
-
-  # throw error if duplicate table names and `repair = check_unique`
-  expect_dm_error(
-    dm(mtcars) %>% copy_to(mtcars, repair = "check_unique"),
-    "need_unique_names"
-  )
-
-  skip_if_not_installed("dbplyr")
-
-  # copying `tibble` from chosen src to sqlite() `dm`
-  expect_equivalent_dm(
-    copy_to(dm_for_filter_sqlite(), data_card_1(), "test_table"),
-    dm_add_tbl(dm_for_filter_sqlite(), test_table = data_card_1_sqlite())
-  )
-
-  # copying sqlite() `tibble` to `dm` on src of choice
-  expect_equivalent_dm(
-    suppress_mssql_message(copy_to(dm_for_filter(), data_card_1_sqlite(), "test_table_1")),
-    dm_add_tbl(dm_for_filter(), test_table_1 = data_card_1())
-  )
-})
-
 test_that("'collect.dm()' collects tables on DB", {
   def <-
     dm_for_filter() %>%

tests/testthat/test-dplyr-src.R

@@ -0,0 +1,92 @@
+test_that("can access tables", {
+  local_options(lifecycle_verbosity = "quiet")
+
+  skip_if_not_installed("nycflights13")
+
+  expect_identical(tbl(dm_nycflights13(), "airlines"), nycflights13::airlines)
+  expect_dm_error(
+    tbl_impl(dm_nycflights13(), "x"),
+    class = "table_not_in_dm"
+  )
+})
+
+test_that("'copy_to.dm()' works", {
+  local_options(lifecycle_verbosity = "quiet")
+
+  expect_dm_error(
+    copy_to(dm_for_filter(), letters[1:5], name = "letters"),
+    "only_data_frames_supported"
+  )
+
+  expect_dm_error(
+    copy_to(dm_for_filter(), list(mtcars, iris)),
+    "only_data_frames_supported"
+  )
+
+  expect_dm_error(
+    copy_to(dm_for_filter(), mtcars, overwrite = TRUE),
+    "no_overwrite"
+  )
+
+  skip_if_src_not("df", "mssql")
+
+  # `tibble()` call necessary, #322
+  car_table <- test_src_frame(!!!mtcars)
+
+  expect_equivalent_dm(
+    suppress_mssql_message(copy_to(dm_for_filter(), mtcars, "car_table")),
+    dm_add_tbl(dm_for_filter(), car_table)
+  )
+
+  # FIXME: Why do we do name repair in copy_to()?
+  expect_equivalent_dm(
+    suppress_mssql_message(expect_name_repair_message(
+      copy_to(dm_for_filter(), mtcars, "")
+    )),
+    dm_add_tbl(dm_for_filter(), ...7 = car_table)
+  )
+})
+
+test_that("'copy_to.dm()' works (2)", {
+  local_options(lifecycle_verbosity = "quiet")
+
+  expect_dm_error(
+    copy_to(dm(), mtcars, c("car_table", "another_table")),
+    "one_name_for_copy_to"
+  )
+
+  # rename old and new tables if `repair = unique`
+  expect_name_repair_message(
+    expect_equivalent_dm(
+      copy_to(dm(mtcars), mtcars),
+      dm(mtcars...1 = mtcars, mtcars...2 = tibble(mtcars))
+    )
+  )
+
+  expect_equivalent_dm(
+    expect_silent(
+      copy_to(dm(mtcars), mtcars, quiet = TRUE)
+    ),
+    dm(mtcars...1 = mtcars, mtcars...2 = tibble(mtcars))
+  )
+
+  # throw error if duplicate table names and `repair = check_unique`
+  expect_dm_error(
+    dm(mtcars) %>% copy_to(mtcars, repair = "check_unique"),
+    "need_unique_names"
+  )
+
+  skip_if_not_installed("dbplyr")
+
+  # copying `tibble` from chosen src to sqlite() `dm`
+  expect_equivalent_dm(
+    copy_to(dm_for_filter_sqlite(), data_card_1(), "test_table"),
+    dm_add_tbl(dm_for_filter_sqlite(), test_table = data_card_1_sqlite())
+  )
+
+  # copying sqlite() `tibble` to `dm` on src of choice
+  expect_equivalent_dm(
+    suppress_mssql_message(copy_to(dm_for_filter(), data_card_1_sqlite(), "test_table_1")),
+    dm_add_tbl(dm_for_filter(), test_table_1 = data_card_1())
+  )
+})

tests/testthat/test-filter-dm.R

@@ -107,7 +107,7 @@ test_that("dm_filter() works as intended for reversed dm", {
     rev(output_1())
   )
 })
-4
+
 test_that("dm_filter() works as intended for inbetween table", {
   expect_equivalent_tbl_lists(
     dm_filter(dm_for_filter(), tf_3, g == "five") %>% dm_apply_filters() %>% dm_get_tables(),

tests/testthat/test-rows-dm.R

@@ -74,6 +74,9 @@ test_that("dm_rows_insert()", {
     # Apply:
     dm_rows_insert(flights_sqlite, flights_feb_sqlite, in_place = TRUE)
     print(dm_nrow(flights_sqlite))
+
+    # Disconnect
+    dbDisconnect(sqlite)
   })
 })
 

tests/testthat/test-zoom.R

@@ -120,7 +120,7 @@ test_that("all cols are tracked in zoomed table", {
     dm_nycflights_small() %>%
       dm_zoom_to(flights) %>%
       get_tracked_cols(),
-    set_names(colnames(tbl(dm_nycflights_small(), "flights")))
+    set_names(colnames(dm_nycflights_small()$flights))
   )
 })