RConsortium / S7

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

Don't inline classes in constructor if possible #481

Open t-kalinowski opened 3 days ago

t-kalinowski commented 3 days ago

This PR changes a constructor default formal value from an inlined class call, to a call like pkgname::classname(), if class@package is not NULL.

closes #477

With this PR:

library(S7)
Coordinate <- new_class(
  "Coordinate",
  properties = list(
    x = class_double,
    y = class_double
  ),
  package = "draw"
)

Shape <- new_class(
  "Shape",
  properties = list(
    position = Coordinate, # would normally inline Coordinate
    name = class_character
  ),
  package = "draw"
)

str(formals(Shape))
#> Dotted pair list of 2
#>  $ position: language draw::Coordinate()
#>  $ name    : chr(0)
stopifnot(identical(
  formals(Shape)$position,
  quote(draw::Coordinate())
))
t-kalinowski commented 3 days ago

This approach of using pkgname::classname() might not work, because new_class() is expected to run at package build time, before package exports are fully known (i.e., before a call like :: or getNamespaceExports() or getExportedValue() will work correctly).

We might have to change the parent environment of the constructor to the environment where new_class() is called. Though, that also brings challenges even in S7 itself, like:

** testing if installed package can be loaded from temporary location
Error: package or namespace load failed for ‘S7’:
 .onLoad failed in loadNamespace() for 'S7', details:
  call: NULL
  error: 'new_object' is not an exported object from 'namespace:S7'
Error: loading failed
lawremi commented 23 hours ago

Qualifying calls by the namespace should only be necessary when referencing class from another package. Would avoiding unnecessary qualification resolve the issue?

hadley commented 8 hours ago

This is starting to feel related to #341

t-kalinowski commented 5 hours ago

OOPWG notes: This should be fixed and merged before release.