RConsortium / S7

S7: a new OO system for R
https://rconsortium.github.io/S7
Other
386 stars 32 forks source link

Unexpected `super()` behavior while trying to dispatch print method #366

Closed calderonsamuel closed 8 months ago

calderonsamuel commented 11 months ago

I want a custom print method for a child class of data.frame. This is a toy example of the class deffinition.

foo_S3 <- function() {
    x <- data.frame()
    class(x) <- c("foo_S3", class(x))
    x
}

foo_S3()
#> data frame with 0 columns and 0 rows

I print some metadata and use NextMethod() to print the data.frame.

print.foo_S3 <- function(x) {
    cli::cli_text("This is a foo_S3 object")
    NextMethod(x)
}

foo_S3()
#> This is a foo_S3 object
#> data frame with 0 columns and 0 rows

While trying to do the same with S7, the class deffinition works ok.

library(S7)

foo_S7 <- new_class("foo_S7", parent = class_data.frame)

foo_S7()
#> data frame with 0 columns and 0 rows

Following the examples on the super() help page, I would expect to define the print method like this, but it looks like super()'s own method takes precedence.

method(print, foo_S7) <- function(x) {
    cli::cli_text("This is a foo_S7 object")
    print(super(x, to = class_data.frame))
}

foo_S7()
#> This is a foo_S7 object
#> super(<foo_S7>, <data.frame>)

Eventually, I would like to inherit from tibble and not from data.frame, which I'm finding very difficult to do, but that will be a separate issue.

Created on 2023-09-19 with reprex v2.0.2

hadley commented 11 months ago

print() is an S3 generic so doesn't understand how to use super(). In this case, I think the right action is to get the underlying S3 object out of your class:

method(print, foo_S7) <- function(x) {
  cli::cli_text("This is a foo_S7 object")
  print(S7_data(x))
}

We need to document this better.

calderonsamuel commented 11 months ago

Using NextMethod() worked for me. Is using S7_data() always a better alternative?

method(print, foo_S7) <- function(x) {
    cli::cli_text("This is a foo_S7 object")
    NextMethod("print", x)
}
hadley commented 11 months ago

Hmmmmmmmmmmmmmmmmmmm.

NextMethod() works because this is an S3 method, and so I think it's fine. But it's problematic for the same reasons that NextMethod() is problematic in S3 code: it's not obvious exactly what method is going to be called. So I think using S7_data() is a bit clearer here, but I don't feel strongly about it.