fixes for S4 generics and classes defined across packages#663
Open
lawremi wants to merge 3 commits into
Open
Conversation
… classes in the package context
… existing functions generic, generic@package is the package of the original function
Member
|
BTW I'm happy to review any of your PRs, but I've been waiting for you to explicitly request a review. |
hadley
reviewed
Jun 1, 2026
| } | ||
|
|
||
| S4_package_name <- function(f, env) { | ||
| if (methods::getPackageName(topenv(env)) == f@package) { |
Member
There was a problem hiding this comment.
Do we need create = FALSE here? (for safety)
| find_package_with_symbol <- function(name, env, exclude = NULL) { | ||
| imports <- getNamespaceImports(topenv(env)) | ||
| pkgs <- setdiff(names(imports), exclude) | ||
| Find( |
Member
There was a problem hiding this comment.
IMO I think this would be a little easier to understand if it used a for loop with an early return when the object was found, and then the error message from above could move into this function.
|
|
||
| # generic was defined for a function from a different package, like base | ||
| find_package_with_symbol(name, env, exclude = f@package) %||% | ||
| stop( |
Member
There was a problem hiding this comment.
Probably worth switching to use stop2() and sprintf() for consistency with other errors in this package.
| return(f@package) | ||
| } | ||
|
|
||
| name <- as.vector(f@generic) |
Member
There was a problem hiding this comment.
Should this be as.character()? But I don't know what f@generic would be otherwise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This contains two fixes:
generic@packageindicates the home package of the generic. Unfortunately, that is not the case in general. The methods package uses@packageto refer to the package defining the original function that the generic represents. Thus,setGeneric("nrow", ...)in a package will create a generic where@package == "base". To make matters worse,topenv(environment(generic))is the namespace of@package, so the only way to find the package defining the generic is a brute force search through namespace imports.getClass()call to resolve S7 old classes, even though that seemed to be the intent. Omittingwheremight problem if a parent package registers those classes with S4.