Skip to content
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

.S3 doesn't work correctly if class contains a dot (".") #86

Closed
bspalat opened this issue Dec 4, 2023 · 4 comments
Closed

.S3 doesn't work correctly if class contains a dot (".") #86

bspalat opened this issue Dec 4, 2023 · 4 comments

Comments

@bspalat
Copy link

bspalat commented Dec 4, 2023

Hello there and thanks for an awesome package!

I stumbled upon the fact that import(..., .S3 = TRUE) doesn't handle S3 methods for classes like data.frame. Apparently, the class doesn't get processed completely which is even hinted at with a warning.
Short example:

# File: module.R
f <- function(x) UseMethod("f")

f.integer <- function(x) TRUE
f.data.frame <- function(x) FALSE

# works here
# f(1L)
# f(data.frame())

but doesn't work when imported:

# File: main.R
import::from(module.R, f, .S3 = TRUE)
# > Warning messages:
# > 1: In assign(paste(genname, class, sep = "."), method, envir = table) :
# >  only the first element is used as variable name
# > 2: In (function (..., deparse.level = 1)  :
# >   number of columns of result is not a multiple of vector length (arg 1)

f(1L)
f(data.frame())
# > Error in UseMethod("f") : 
# > no applicable method for 'f' applied to an object of class "data.frame"

methods(f)
# > [1] f.data*    f.integer*

The feature is labeled as experimental and the above isn't difficult to circumvent, but I suppose the behaviour isn't intentional.

Once again, thanks for all the work :)

@torfason
Copy link
Collaborator

torfason commented Dec 5, 2023

Thanks for the report. This clearly has to do with how R resolves ambiguous (more than one period) names into <method>.<class>. Definitely not intentional behavior, and if someone were to contribute a fix in a PR, along with unit tests, I would try to merge it and possibly roll a bugfix release (say 1.3.2). But I personally don't know exactly how to fix this and would probably have to spend some time to figure that out, so I'm unlikely to find the time myself, unfortunately.

@J-Moravec
Copy link
Contributor

J-Moravec commented Jan 16, 2024

Ah, I was wondering why the S3 support failed on me recently and went back to registering manually.

I will research it and fix it.

But note that there might be no way to distinguish between the hypothetical cases mentioned in #65 (comment)

A hypothetical t.test.data.frame might then consist of t generic for the test.data.frame class, t.test generic for the data.frame class and t.test.data generic for the frame class and there is no easy way to resolve this ambiguity.

J-Moravec added a commit to J-Moravec/import that referenced this issue Jan 18, 2024
@J-Moravec
Copy link
Contributor

I was able to find a bug in my original implementation.

Note that we are still assuming that the generics doesn't have a dot in name. This is probably a reasonable assumption for new user-generated generics, but some historical generics might suffer. Users can always identify and register these issues manually.

@torfason
Copy link
Collaborator

Awesome, thanks for the fix, we should see this on CRAN within a day or two, se the PR (#87) for detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants