Use rlang dots helper instead of ellipsis

[romainfrancois]

Perhaps this should drop the rlang:: prefix too since rlang is @import'ed

[lionel-]

I plan to deprecate the action = , maybe wait until that's the case? I'll do it next week.

[lionel-]

Why is this not throwing errors?

[romainfrancois]

cc @krlmlr

[krlmlr]

These methods are unusual in the sense that their main purpose may be the side effects they cause, especially with in_place = TRUE for database backends. Throwing an error after the side effects happened would be wrong here.

[DavisVaughan]

I'm generally in favor of removing the check_dots_*() helpers from the generics, and letting the methods call check_dots_empty(), like what @romainfrancois did in dm cynkra/dm#639

The only other place we use check_dots_used() like this is in pull(), but we could change that too to make pull.data.frame() call check_dots_empty().

[romainfrancois]

Good catch, perhaps we can do pull() in a follow up pull request. Although it looks like more methods exist:
https://github.com/search?q=org%3Acran+S3method%28pull%2C&type=code

So perhaps more implementations would have to use check_dots_empty()

[lionel-]

Perhaps this should drop the rlang:: prefix too since rlang is @import'ed

yeah for consistency

[romainfrancois]

Not sure why still getting

W  checking dependencies in R code (1.3s)
   '::' or ':::' import not declared from: ‘ellipsis’

[DavisVaughan]

Ooooh @romainfrancois this was a tough one. It is because we inline tidyselect::peek_vars here and that uses ellipsis::

dplyr/R/select-helpers.R

Line 53 in 0445420

peek_vars <- tidyselect::peek_vars

Found by stepping through tools:::.check_packages_used("dplyr")

It would be great to remove that if we can, I think

[romainfrancois]

This seems to do the trick. Thanks @DavisVaughan. cc @lionel- in case peek_vars <- tidyselect::peek_vars is still needed for something, I don't believe so.

Perhaps we can use check_dots_empty() on the rows_*.data.frame methods and let implementations (in dm:: ?) do something different with the ... ? cc @krlmlr

[romainfrancois]

We have:

rows_insert <- function(x, y, by = NULL, ..., copy = FALSE, in_place = FALSE) {
  # Need action = warn, because methods may have
  # side effects that persist even after we abort
  check_dots_used()
  UseMethod("rows_insert")
}

and then in dm::

rows_insert.tbl_dbi <- function(x, y, by = NULL, ...,
                                in_place = NULL, copy = FALSE, check = NULL,
                                returning = NULL) {

And the ... are not passed through, so this is just about checking that specific arguments of the methods (check=, returning= are fully named ? Should dm rather itself call check_dots_empty() so that it fails when that happens rather warn ?

[krlmlr]

Am I understanding this right: if all methods use check_dots_empty(), we don't need check_dots_used() in the generic?

[romainfrancois]

Yeah I guess so, at least in this case where IIUC the generic had check_dots_used() to make sure everything in the ... was used after the end of the methods, which as the comment implied might be too late, happen after some changes have been performed in the database, ....

If instead, the method checks that the ... are unused, it can fail immediately, and so avoid doing unwanted work on the database.

[lionel-]

Another potential reason for using check_dots_empty() in dplyr methods instead of check_dots_used() in dplyr generics is that the latter can't detect that an argument was defused with {{ or enquo().