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

Allow "seed" arguments to constructors #420

Closed
lawremi opened this issue Aug 15, 2024 · 4 comments · Fixed by #444
Closed

Allow "seed" arguments to constructors #420

lawremi opened this issue Aug 15, 2024 · 4 comments · Fixed by #444
Milestone

Comments

@lawremi
Copy link
Collaborator

lawremi commented Aug 15, 2024

Related to #376 but deserves its own issue.

One problem with default constructors right now is that they do not take their first argument to be an instance of the class, or superclass, to be used as a basis for construction. Experience in S4 shows that it is extremely useful to be able to add properties on top of an instance of the superclass to create an instance of the subclass.

For example:

Foo <- new_class("Foo", properties = list(x = class_numeric))
Bar <- new_class("Bar", Foo, properties = list(y = class_numeric))
foo <- Foo(x = 1)
bar <- Bar(foo, y = 2)

Since this is close to the behavior of new_object() now, it obviously wouldn't be hard to support this. User-specified constructors already need to follow this convention (I was surprised to find that the default constructor did not). The default constructor Bar would look something like this:

function(.parent = Foo(x = x), x, y)
new_object(.parent, x = x, y = y)

Classes that inherit from a base class already have this feature, since they get a .data argument. In those cases, .data would be generalized to .parent.

This change would break compatibility though, because people may be passing unnamed arguments to the constructor (which I would consider pretty bad practice). Code should not be making assumptions about the order of the properties in the object (and thus constructor). It just seems too brittle. Users should expect things to break at this stage.

If we really wanted to avoid breaking compatibility, we could just add the .parent argument at the end. The caller would likely have to name .parent in the call, but the feature would at least exist.

We could even go a step further and support merging of all unnamed arguments passed to the constructor, like the methods package does. Related to #409, new_object() does not check for whether it has unnamed arguments in .... If we're not going to support unnamed arguments, then there should be an informative error.

@lawremi lawremi changed the title Allowing unnamed arguments to constructors Allow "seed" arguments to constructors Aug 15, 2024
@t-kalinowski
Copy link
Member

As I implement this feature, an interesting design question has come up: How should we handle cases where a user provides both a .seed and explicit parent properties?

One approach could be to allow both, treating the explicit properties as overrides to the .seed. This would require delaying parent validation until after setting any "override" properties. I'm considering two ways to achieve this:

Using ... to pass additional properties:

function(.seed = Foo@constructor(), y = numeric(), ...) {
  parent_props <- list(...)
  for (i in seq_along(parent_props)) {
    prop(.seed, names(parent_props)[i], check = FALSE) <- parent_props[[i]]
  }
  new_object(.seed, y = y)
}

Or including parent properties as missing arguments:

function(.seed = Foo@constructor(), y = numeric(), x) {
  if (!missing(x)) {
    prop(.seed, "x", check = FALSE) <- x
  }
  new_object(.seed, y = y)
}

What do you think about these approaches? Are there other solutions we should consider?

@t-kalinowski t-kalinowski added this to the v0.2.0 milestone Sep 16, 2024
@t-kalinowski
Copy link
Member

t-kalinowski commented Sep 16, 2024

Meeting notes:

  • The first sketched approach is preferable.
  • A better name than .parent might be .proto.

@t-kalinowski
Copy link
Member

t-kalinowski commented Sep 16, 2024

We will also need to rename the accessor S7_data() to match.

@t-kalinowski
Copy link
Member

t-kalinowski commented Sep 18, 2024

One potential drawback of this change is that parent arguments become less transparent when placed within .... I'm hesitant to make the user-facing interface more opaque.

While we could potentially address some issues that ... brings through tooling (e.g., using @inheritDotParams and relying on IDE tools to resolve property names for autocomplete), I'm wondering if there might be a more straightforward solution.

What do you think about introducing a separate method for this? Here's how it could work:

Foo <- new_class("Foo", properties = list(x = class_numeric))
Bar <- new_class("Bar", Foo, properties = list(y = class_numeric))

# Constructor signatures would remain clear:
# formals(Foo) == alist(x = integer())
# formals(Bar) == alist(x = integer(), y = integer())

foo <- Foo(x = 1)
bar <- Bar@from_parent(foo, y = 2)

# The new method would have an explicit .parent parameter:
# formals(Bar@from_parent) == alist(.parent = Foo(), y = integer(), ...)

Another approach could be to use convert() for this purpose (as proposed in #430):

foo <- Foo(x = 1)
bar <- convert(foo, to = Bar, y = 2)

What do you think about these alternatives?

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

Successfully merging a pull request may close this issue.

2 participants