Enhance OO mocking#2303
Conversation
Fixes #2302
|
Claude finished @hadley's task —— View job PR Review: Enhance OO mockingReviewed the OO mocking enhancements. Found one potential issue: Issue in if (env_has(s3_envir, ".__S3MethodsTable__.")) {
table <- env_get(s3_envir, ".__S3MethodsTable__.")
env_unbind(table, paste0(generic, ".", class))
}The function should check if the method exists in the table before attempting to unbind it. Calling Suggested fix: if (env_has(s3_envir, ".__S3MethodsTable__.")) {
table <- env_get(s3_envir, ".__S3MethodsTable__.")
method_name <- paste0(generic, ".", class)
if (env_has(table, method_name)) {
env_unbind(table, method_name)
}
}Otherwise, the changes look solid with good test coverage. |
| check_function(definition) | ||
| check_function(definition, allow_null = TRUE) | ||
|
|
||
| generic_def <- get(generic, envir = frame) |
| genfun <- get(generic, envir = envir) | ||
| s3_envir <- environment(genfun) %||% .BaseNamespaceEnv |
There was a problem hiding this comment.
i noticed that registerS3Method is slightly safer here, checking typeof(genfun) first before calling environment() on it
genfun <- get(genname, envir = envir)
....
if (typeof(genfun) == "closure")
environment(genfun)
else .BaseNamespaceEnv(Maybe environment() never fails, even with weird input, im not 100% sure)
There was a problem hiding this comment.
Yeah, I skipped that because the environment of a "special" is NULL.
| check_function(definition, allow_null = TRUE) | ||
|
|
||
| generic_def <- get(generic, envir = frame) | ||
|
|
There was a problem hiding this comment.
I noticed that the S4 bit checks that the generic exists first, but the S3 bit does not. Should we enforce that the S3 generic exists before doing any work?
There was a problem hiding this comment.
I don't think it's worth it, as there's no simpler equivalent of getGeneric() for S3 generics and the existing error message is ok.
| set_method <- function(def) { | ||
| env <- topenv(frame) | ||
| old <- methods::getMethod(generic_def, signature, optional = TRUE) | ||
| if (is.null(def)) { | ||
| methods::removeMethod(generic_def, signature, env) | ||
| } else { | ||
| suppressMessages(methods::setMethod(generic_def, signature, def, env)) | ||
| } | ||
| old | ||
| } | ||
|
|
||
| old <- set_method(definition) | ||
| withr::defer(set_method(old), frame) |
There was a problem hiding this comment.
Was it purposeful that set_method() locally captures generic_def and signature from the surrounding env rather than making them arguments? That kind of local capture always feels like a code smell to me (or when I need it I drop a comment calling it out). I would have rather seen set_method(generic_def, signature, def) at each call site, where you see the full set of info required to set the method.
At the very least, I had to think about it for a minute when I saw generic_def in there but didn't see it defined anywhere, and had to backtrack up the outer function to find it.
There was a problem hiding this comment.
Yes, because otherwise the calls are too long to fit nicely on one line. But maybe I refactored more since that was a problem.
You don't think having a function definition inside another function is a strong indicator that you're using local state?
There was a problem hiding this comment.
You don't think having a function definition inside another function is a strong indicator that you're using local state
I guess that's true, I just see it so rarely that it trips my brain up for a minute
There was a problem hiding this comment.
Making the arguments explicit no longer causes unpleasant wrapping, so I've made the change.
| if (is.null(def)) { | ||
| methods::removeMethod(generic_def, signature, env) | ||
| } else { | ||
| suppressMessages(methods::setMethod(generic_def, signature, def, env)) |
There was a problem hiding this comment.
We were not suppressing anything before, what are we...uh...suppressing?
| #' length(x) | ||
| local_mocked_s3_method <- function( | ||
| generic, | ||
| signature, |
There was a problem hiding this comment.
I am mildly irked this was called signature and not class, since class is more S3 like
| test_that("can temporarily remove S3 method with NULL", { | ||
| x <- structure(list(), class = "test_mock_class2") | ||
|
|
||
| local({ | ||
| local_mocked_s3_method("length", "test_mock_class2", function(x) 42) | ||
| expect_length(x, 42) | ||
|
|
||
| # Now remove it | ||
| local_mocked_s3_method("length", "test_mock_class2", NULL) | ||
| expect_length(x, 0) | ||
| }) | ||
|
|
||
| # Method should be removed after scope ends | ||
| expect_length(x, 0) | ||
| }) |
There was a problem hiding this comment.
What about a test where length.test_mock_class2 does already exists before entering the local(), then you temporarily remove it for the local() scope, and then after exiting the local scope you ensure that the old definition was restored?
There was a problem hiding this comment.
it actually looks like you do have a test for this for s4 with mock_age()
There was a problem hiding this comment.
Ah yeah, this test doesn't really focus on the removing. I'll improve.
2 participants
Fixes #2302