RConsortium / S7

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

`print.S7_object()` invokes custom property `getter` #451

Closed t-kalinowski closed 2 weeks ago

t-kalinowski commented 1 month ago
library(S7)
Foo <- new_class("Foo", properties = list(
    double = new_property(
        class_double,
        getter = function(self) stop("not a double")
    )
))

foo <- Foo()

foo
#> <Foo>
#> Error in (function (self) : not a double

We probably should not call getter() from the print method, as it might be a potentially expensive operation.

lawremi commented 1 month ago

To me, the question is more about what constitutes the structure, str(), of the object? Are dynamic properties part of the structure? Users will not care about whether a property is dynamic or not; they just see them as properties, so I think the answer is "yes." It would be surprising for a property not to show up in str().

This reminds of the deprecated property case though. It would be annoying to get a warning every time the object is printed, so there would need to be a custom str() method. Or, we add formal support for property deprecation that effectively hides them.

mmaechler commented 4 weeks ago

Possibly both are right: str() should mention properties by default. OTOH, its S7_object method get an option to suppress them. Then the print() method may in some cases use that suppression option ..

t-kalinowski commented 3 weeks ago

I think we can default to calling print() with suppressWarnings(), to handle the deprecated property case.

t-kalinowski commented 2 weeks ago

For the deprecated property case, if class authors truly want to deprecate a property, they'll also want to implement a print() method that doesn't show the deprecated property. Alternatively, they can include conditional logic in the getter to suppress warnings when called from print, like this:

getter = function(self) {
  warn <- TRUE
  for (cl in sys.calls()) {
    if (cl[[1]] == quote(print.S7_object)) {
      warn <- FALSE
      break
    }
  }
  if (warn) {
    warning("@foo is deprecated")
  }
  self@newfoo
}