search

r-lib / R6

Encapsulated object-oriented programming for R
https://R6.r-lib.org
Other
410 stars 56 forks source link

Subclasses should inherit value of 'cloneable' by default #165

Closed wch closed 1 year ago

gaborcsardi commented 5 years ago

I think it is fine to have the default still FALSE, even if the parent class is cloneable. In the subclass you might add tags that make the class non-cloneable, it is safer to require an explicit cloneable = TRUE in the subclass as well, imo.

(Btw. I know this is unlikely, but if we ever get multiple inheritance, then that will not interact well with this change.)

wch commented 5 years ago

The problem is that the default is TRUE. In retrospect, the default should have been FALSE, but it's too late to make that change now.

gaborcsardi commented 5 years ago

Oh, right, I forgot that. :(

wch commented 5 years ago

For multiple inheritance, maybe it could take the AND of the parents' values for cloneable by default.

gaborcsardi commented 5 years ago

Yeah, that would be reasonable.

s-fleck commented 5 years ago

I'm not sure if this fits here, but I did not want to add a new issue since it seems related: If I inherit from a clonable object, and set clonable = FALSE in the child, the child sill has a clone() method. Is this desired behaviour?

mmuurr commented 3 years ago

+1 to @s-fleck's question. I recently discovered this, too. That a subclass with cloneable = FALSE inherits its parent's clone() method is surprising to me.

My default assumption would be that explicitly setting cloneable = FALSE would remove that method. Though I also see how this can cause confusion for say, a parent class that has two child extenders, one that is cloneable and one that isn't ... any user of the parent class can't reliably now know if the instance is cloneable (save for a runtime check).

A counter-example pattern where this is confusing in the other direction:

Parent <- R6::R6Class("Parent")
Child <- R6::R6Class("Child", inherit = Parent, cloneable = FALSE)
tot <- Child$new()
tot$clone()  ## works (inherited from Parent), but feels like it shouldn't, because we said so in the Generator

Of the two potential sources of confusion/surprisal, the second seems tougher to justify: the class author explicitly asks for cloneable = FALSE and yet clone() exists.