For *_join functions, requesting handling when argument "on" is specified in place of "by" #6228

Activity

1. DavisVaughan commented on Apr 7, 2022

   

   DavisVaughan

   on Apr 7, 2022

   Member

   More actions

   Minimal reprex of the general idea that our join functions swallow ... silently

   library(dplyr)
   
   left_join(
     band_instruments,
     band_members,
     by = "name",
     NONEXISTENT = "ARG"
   )
   #> # A tibble: 3 × 3
   #>   name  plays  band   
   #>   <chr> <chr>  <chr>  
   #> 1 John  guitar Beatles
   #> 2 Paul  bass   Beatles
   #> 3 Keith guitar <NA>

   @hadley do you think it would be reasonable for the data frame methods to call check_dots_empty()? I have a feeling putting any checks in the generic at this point is going to cause issues, but it feels safe and useful to put a check in the data frame method.
2. hadley commented on Apr 8, 2022

   

   hadley

   on Apr 8, 2022

   Member

   More actions

   @DavisVaughan can we use check_dots_used() in the generic?
3. DavisVaughan commented on Apr 8, 2022

   

   DavisVaughan

   on Apr 8, 2022

   Member

   More actions

   I'll give that a try and see if revdeps flags anything. That seems reasonable.

   Details

   Convincing myself that check_dots_used() works as we want:

   fn <- function(x, ...) {
     rlang::check_dots_used()
     UseMethod("fn")
   }
   
   fn.default <- function(x, ...) {
     list(x)
   }
   
   fn.double <- function(x, ..., option = NULL) {
     list(x, option)
   }
   
   # error because `option` isnt a valid argument to that method
   fn(1L, option = 2)
   #> Error:
   #> ! Arguments in `...` must be used.
   #> x Problematic argument:
   #> • option = 2
   
   # fine
   fn(1, option = 2)
   #> [[1]]
   #> [1] 1
   #> 
   #> [[2]]
   #> [1] 2
   
   # typo
   fn(1, options = 2)
   #> Error:
   #> ! Arguments in `...` must be used.
   #> x Problematic argument:
   #> • options = 2

   VS check_dots_empty() which is nice in the method but not nice in the generic

   fn <- function(x, ...) {
     rlang::check_dots_empty()
     UseMethod("fn")
   }
   
   fn.double <- function(x, ..., option = NULL) {
     list(x, option)
   }
   
   # not good for us, we want to allow this
   fn(1, option = 2)
   #> Error in `fn()`:
   #> ! `...` must be empty.
   #> x Problematic argument:
   #> • option = 2

   check_dots_used() does have this slightly unfortunate behavior though

   fn <- function(x, ...) {
     rlang::check_dots_used()
     UseMethod("fn")
   }
   
   fn.integer <- function(x, ..., option = NULL) {
     list(x, option)
   }
   
   fn.double <- function(x, ..., option = NULL) {
     list(x)
   }
   
   # Actually evaluates the `option` promise so no issues
   fn(1L, option = 2)
   #> [[1]]
   #> [1] 1
   #> 
   #> [[2]]
   #> [1] 2
   
   # Doesn't evaluate the `option` promise, but we do have an `option` argument.
   # Since the promise isn't evaluated it looks "unused"
   fn(1, option = 2)
   #> Error:
   #> ! Arguments in `...` must be used.
   #> x Problematic argument:
   #> • option = 2
4. hadley

   added

   featurea feature request or enhancementa feature request or enhancement

   tables 🧮joins and set operationsjoins and set operations

   on Apr 15, 2022
5. hadley

   added a commit that references this issue on Jul 22, 2022

   Join functions check for extra arguments

   ...

   This issue will close once commit 4acc8ec is merged into the 'main' branch.

   4acc8ec
6. hadley

   closed this as completedin #6353on Jul 26, 2022
7. hadley

   added a commit that references this issue on Jul 26, 2022

   Check for extra arguments in join functions (#6353)

   ...

   This issue will close once commit 17c7a5d is merged into the 'main' branch.

   17c7a5d