R devel - Aug 2023 - Improving user-friendliness of S4 dispatch failure when mis-naming arguments?

Here's a trivial patch that offers some improvement:

Index: src/library/methods/R/methodsTable.R
==================================================================---
src/library/methods/R/methodsTable.R (revision 84931)
+++ src/library/methods/R/methodsTable.R (working copy)
@@ -752,11 +752,12 @@
   if(length(methods) == 1L)
     return(methods[[1L]]) # the method
   else if(length(methods) == 0L) {
-    cnames <- paste0("\"", vapply(classes, as.character,
""), "\"",
+    cnames <- paste0(head(fdef at signature, length(classes)),
"=\"",
vapply(classes, as.character, ""), "\"",
      collapse = ", ")
     stop(gettextf("unable to find an inherited method for function %s
for signature %s",
                   sQuote(fdef at generic),
                   sQuote(cnames)),
+         call. = FALSE,
          domain = NA)
   }
   else

Here's the upshot for the example on DBI:

dbGetQuery(connection = conn, query = query)
Error: unable to find an inherited method for function ?dbGetQuery?
for signature ?conn="missing", statement="missing"?

I don't have any confidence about edge cases / robustness of this
patch for generic S4 use cases (make check-all seems fine), but I
don't suppose a full patch would be dramatically different from the
above.

Mike C

On Thu, Aug 10, 2023 at 12:39?PM Gabriel Becker <gabembecker at gmail.com>
wrote:
>
> I just want to add my 2 cents that I think it would be very useful and
beneficial to improve S4 to surface that information as well.
>
> More information about the way that the dispatch failed would be of great
help in situations like the one Michael pointed out.
>
> ~G
>
> On Thu, Aug 10, 2023 at 9:59?AM Michael Chirico via R-devel <r-devel at
r-project.org> wrote:
>>
>> I forwarded that along to the original reporter with positive feedback
>> -- including the argument names is definitely a big help for cuing
>> what exactly is missing.
>>
>> Would a patch to do something similar for S4 be useful?
>>
>> On Thu, Aug 10, 2023 at 6:46?AM Hadley Wickham <h.wickham at
gmail.com> wrote:
>> >
>> > Hi Michael,
>> >
>> > I can't help with S4, but I can help to make sure this
isn't a problem
>> > with S7. What do you think of the current error message? Do you
see
>> > anything obvious we could do to improve?
>> >
>> > library(S7)
>> >
>> > dbGetQuery <- new_generic("dbGetQuery",
c("conn", "statement"))
>> > dbGetQuery(connection = NULL, query = NULL)
>> > #> Error: Can't find method for generic `dbGetQuery(conn,
statement)`:
>> > #> - conn     : MISSING
>> > #> - statement: MISSING
>> >
>> > Hadley
>> >
>> > On Wed, Aug 9, 2023 at 10:02?PM Michael Chirico via R-devel
>> > <r-devel at r-project.org> wrote:
>> > >
>> > > I fielded a debugging request from a non-expert user today.
At root
>> > > was running the following:
>> > >
>> > > dbGetQuery(connection = conn, query = query)
>> > >
>> > > The problem is that they've named the arguments
incorrectly -- it
>> > > should have been [1]:
>> > >
>> > > dbGetQuery(conn = conn, statement = query)
>> > >
>> > > The problem is that the error message "looks"
highly confusing to the
>> > > untrained eye:
>> > >
>> > > Error in (function (classes, fdef, mtable)  :   unable to
find an
>> > > inherited method for function ?dbGetQuery? for signature
?"missing",
>> > > "missing"?
>> > >
>> > > In retrospect, of course, this makes sense -- the mis-named
arguments
>> > > are getting picked up by '...', leaving the required
arguments
>> > > missing.
>> > >
>> > > But I was left wondering how we could help users right their
own ship here.
>> > >
>> > > Would it help to mention the argument names? To include some
code
>> > > checking for weird combinations of missing arguments? Any
other
>> > > suggestions?
>> > >
>> > > Mike C
>> > >
>> > > [1]
https://github.com/r-dbi/DBI/blob/97934c885749dd87a6beb10e8ccb6a5ebea3675e/R/dbGetQuery.R#L62-L64
>> > >
>> > > ______________________________________________
>> > > R-devel at r-project.org mailing list
>> > > https://stat.ethz.ch/mailman/listinfo/r-devel
>> >
>> >
>> >
>> > --
>> > http://hadley.nz
>>
>> ______________________________________________
>> R-devel at r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel

>>>>> Michael Chirico via R-devel 
>>>>>     on Thu, 10 Aug 2023 23:56:45 -0700 writes:
> Here's a trivial patch that offers some improvement:
    > Index: src/library/methods/R/methodsTable.R
    > ==================================================================   
> --- src/library/methods/R/methodsTable.R (revision 84931)
    > +++ src/library/methods/R/methodsTable.R (working copy)
    > @@ -752,11 +752,12 @@
    > if(length(methods) == 1L)
    > return(methods[[1L]]) # the method
    > else if(length(methods) == 0L) {
    > -    cnames <- paste0("\"", vapply(classes,
as.character, ""), "\"",
    > +    cnames <- paste0(head(fdef at signature, length(classes)),
"=\"",
    > vapply(classes, as.character, ""), "\"",
    > collapse = ", ")
    > stop(gettextf("unable to find an inherited method for function %s
    > for signature %s",
    > sQuote(fdef at generic),
    > sQuote(cnames)),
    > +         call. = FALSE,
    > domain = NA)
    > }
    > else

    > Here's the upshot for the example on DBI:

    > dbGetQuery(connection = conn, query = query)
    > Error: unable to find an inherited method for function ?dbGetQuery?
    > for signature ?conn="missing", statement="missing"?

    > I don't have any confidence about edge cases / robustness of this
    > patch for generic S4 use cases (make check-all seems fine),

Good you checked, but you are right that that's not all enough to be sure:

Checking error *messages* is not something we do often {not
the least because you'd need to consider message translations
and hence ensure you only check in case of English ...}.

    > but I don't suppose a full patch would be dramatically different
from the
    > above.

I agree: The patch looks to make sense to me, too,
while I'm not entirely sure about the extra   call. = FALSE
 (which I of course understand you'd prefer for the above example)

Now I'd like to find an example that only uses packages with priority
 base | Recommended

Martin

--
Martin Maechler
ETH Zurich  and  R Core team


    > Mike C

    > On Thu, Aug 10, 2023 at 12:39?PM Gabriel Becker <gabembecker at
gmail.com> wrote:
    >> 
    >> I just want to add my 2 cents that I think it would be very useful
and beneficial to improve S4 to surface that information as well.
    >> 
    >> More information about the way that the dispatch failed would be of
great help in situations like the one Michael pointed out.
    >> 
    >> ~G
    >> 
    >> On Thu, Aug 10, 2023 at 9:59?AM Michael Chirico via R-devel
<r-devel at r-project.org> wrote:
    >>> 
    >>> I forwarded that along to the original reporter with positive
feedback
    >>> -- including the argument names is definitely a big help for
cuing
    >>> what exactly is missing.
    >>> 
    >>> Would a patch to do something similar for S4 be useful?
    >>> 
    >>> On Thu, Aug 10, 2023 at 6:46?AM Hadley Wickham <h.wickham at
gmail.com> wrote:
    >>> >
    >>> > Hi Michael,
    >>> >
    >>> > I can't help with S4, but I can help to make sure this
isn't a problem
    >>> > with S7. What do you think of the current error message?
Do you see
    >>> > anything obvious we could do to improve?
    >>> >
    >>> > library(S7)
    >>> >
    >>> > dbGetQuery <- new_generic("dbGetQuery",
c("conn", "statement"))
    >>> > dbGetQuery(connection = NULL, query = NULL)
    >>> > #> Error: Can't find method for generic
`dbGetQuery(conn, statement)`:
    >>> > #> - conn     : MISSING
    >>> > #> - statement: MISSING
    >>> >
    >>> > Hadley
    >>> >
    >>> > On Wed, Aug 9, 2023 at 10:02?PM Michael Chirico via
R-devel
    >>> > <r-devel at r-project.org> wrote:
    >>> > >
    >>> > > I fielded a debugging request from a non-expert user
today. At root
    >>> > > was running the following:
    >>> > >
    >>> > > dbGetQuery(connection = conn, query = query)
    >>> > >
    >>> > > The problem is that they've named the arguments
incorrectly -- it
    >>> > > should have been [1]:
    >>> > >
    >>> > > dbGetQuery(conn = conn, statement = query)
    >>> > >
    >>> > > The problem is that the error message
"looks" highly confusing to the
    >>> > > untrained eye:
    >>> > >
    >>> > > Error in (function (classes, fdef, mtable)  :  
unable to find an
    >>> > > inherited method for function ?dbGetQuery? for
signature ?"missing",
    >>> > > "missing"?
    >>> > >
    >>> > > In retrospect, of course, this makes sense -- the
mis-named arguments
    >>> > > are getting picked up by '...', leaving the
required arguments
    >>> > > missing.
    >>> > >
    >>> > > But I was left wondering how we could help users
right their own ship here.
    >>> > >
    >>> > > Would it help to mention the argument names? To
include some code
    >>> > > checking for weird combinations of missing arguments?
Any other
    >>> > > suggestions?
    >>> > >
    >>> > > Mike C
    >>> > >
    >>> > > [1]
https://github.com/r-dbi/DBI/blob/97934c885749dd87a6beb10e8ccb6a5ebea3675e/R/dbGetQuery.R#L62-L64