Skip to content

Include value in error message about invalid value from register_method() #460

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.

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

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

t-kalinowski
Copy link
Member

Closes #447

@hadley
Copy link
Member

hadley commented Oct 11, 2024

Hmmmm, I wonder if we should add a error wrapper in method<- since it has a bit more context. I think ideally I'd want

foo <- function() {}
method(foo, class_character) <- function(...) {}
#> Error: Failed to register <character> method for <foo> generic
#> `generic` is a function, but not an S3 generic function

But looking at the traceback, maybe we can't get the name of the generic because of the *tmp* substitution?

@t-kalinowski
Copy link
Member Author

I don't think it's possible to get the original expression for x in a call like foo(x) <- through introspection.

> `foo<-` <- function(x, value) {
+   print(sys.calls())
+ }
> 
> x <- function() {}
> 
> foo(x) <- "some val"
[[1]]
`foo<-`(`*tmp*`, value = "some val")

@hadley
Copy link
Member

hadley commented Oct 16, 2024

Yeah, so the best we could do is mention the method. Maybe something like this?

foo <- function() {}
method(foo, class_character) <- function(...) {}
#> Error: Failed to register <character> method for generic
#> `generic`, function(...) is a function, but not a generic

R/generic-spec.R Outdated
@@ -28,7 +28,8 @@ as_S3_generic <- function(x) {
}
}

stop("`generic` is a function, but not an S3 generic function", call. = FALSE)
stop("`generic` is a function, but not an S3 generic function: ",
deparse1(x, width.cutoff = 100L), call. = FALSE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think displaying the arguments, without the body, would be sufficient. (Like args(mean) but without the NULL).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function (x, ...) doesn't seem like a very useful breadcrumb, especially if we can easily include the head of the first expression in the function body.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm worried about this spitting out a very large function body, making it hard to see the actual error message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

width.cutoff = 100L

maybe we truncate this more/better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that just determines the line length?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL! (I had thought width.cutoff capped nchar() of the returned string)

Maybe something like:

deparse_trunc <- function(x, width, collapse = "\n") {
  x <- deparse1(x, collapse)
  if (nchar(x)> width) 
     x <- sprintf("%s....", substr(x, 0, width-4))
  x
}

which gives:

> cat(deparse_trunc(plot.default, 80))
function (x, y = NULL, type = "p", xlim = NULL, ylim = NULL, log = "", main ....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that sounds good to me.

@t-kalinowski t-kalinowski merged commit 6e2e582 into main Oct 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

register_method() error is missing context
2 participants