Add clone() function

[wch]

Closes #27. This adds a clone() function for cloning objects, with a few limitations. Some of them should be fixed before merging.

• Doesn't clone an object's super object. This should be fixable without any trouble.
• Deep copy currently isn't supported. Can be fixed, perhaps by looking for a $deepCopy method, similar to #57.
• Active bindings are lost. Fixing this would be a little complicated -- although R can tell you whether a binding is active, it won't give you the function used for an active binding. So it would require keeping separate references to functions used for all of the active bindings. I'm not sure it's worth it.

To create a class that has $clone() method, you can simply add a public method like this:

  clone = function() clone(self)

(Or R6::clone(self), if your package doesn't import R6.)

[thomasp85]

I'm curious to why you prefer this approach to #57 (I don't feel any anger/rejection - just plain curious) You list a set of features that are missing - some of them you don't expect to be directly possible - all of these are taken care of with #57...

[wch]

A big part of it is that I found that I had actually written a lot of this code 9 months ago. (!)

I think the main difference is that in your approach, the $copy method (which is added to every object) contains an implicit reference back to the generator object, via ancestor environments of $copy. That's how you're able to access information about the class from within that function.

With my approach, the objects don't retain any references to the generator object. I think this makes it cleaner -- presently, the environment diagrams (in doc_extra/) fully describe the structure of an R6 object. With your approach, each diagram would include one more binding, copy, that points indirectly to the rather large generator object, so the objects carry around this baggage. One thing I like about R6 as it presently is designed is that the resulting objects are simply environments structured in a certain way. One downside of my approach is that it's not possible to copy active bindings without some added complexity (the other two issues can be easily fixed).

[thomasp85]

Those are valid points. I would advice strongly against foregoing active bindings in the cloning process as this would lead to a lot of confusions for users - it should be possible to do some introspection with bindingIsActive() on the original object to get this done without too much overhead and complexity.

If you merge this into a new branch I'll be happy to contribute...

[wch]

It is possible to tell whether a binding is active, but it's not possible (as far as I know) to actually retrieve the function that's used for the active binding. I agree, though, that it would be confusing for cloning to just not work correctly when there are active bindings.

I think maybe the best strategy is, in each object, to store (in an attribute) a list of the active binding functions. Also, I think it would be good to add a cloneable=TRUE option to R6Class(), so that if people want to use minimal memory and time, they can.

I'll probably do this myself, but if you'd like to make an attempt at implementing active binding copying (without storing a copy of the functions) you can simply check out my clone branch and work off of that.

[wch]

Actually, I just found that as.list.environment returns active bindings as functions, not as the values, so cloning active bindings should be straightforward.

[thomasp85]

Was just going to say that but you beat me to it...

Den 22/06/2015 kl. 18.25 skrev Winston Chang notifications@github.com:

Actually, I just found that as.list.environment returns active bindings as functions, not as the values, so cloning active bindings should be straightforward.

— Reply to this email directly or view it on GitHub.

[wch]

OK, I think it's ready to go - no deep copy functionality yet, but I think that deserves its own PR.

[wch]

I've thought about this a bit more, and now I think it's better not to have a standalone clone() function - it should be embedded in the object. This is because it's possible for the object structure to change between versions (along with the corresponding clone() function), and so using clone() from one version of R6 on an object generated by a different version of R6 could result in errors.

Here's an example: ggplot2 is going to be converted to R6. If you build and install ggplot2 using R6 2.1, and then later upgrade R6 to 2.2, this could potentially break ggplot2, if the clone() function changes. To make ggplot2 work again, you'd have to build it against R6 2.2.

A similar situation has happened in the past with Ref Classes. I believe it was the change from R 3.1.0 to 3.1.1.

Adding the clone() method to each object will avoid this problem. I'm going to do it in a way that avoids saving a reference to the generator, though.

[thomasp85]

I would expect that you could just substitute the new clone method for my copy method and then more or less add the clone method to every object in the same way (maybe letting the developer decide using the clonable argument...) as I did in #57.

The part about ggplot2 is that just a thought experiment or a real aim? I'm struggling to implement a custom geom with a custom grob and if proto is going to be ditched soon I might hold it off (#1044 in hadley/ggplot2).

[wch]

Yup, it's going to be added to each object, and there's going to be a cloneable option.

The change to ggplot2 is a real thing. The plan is for ref classes and proto to go away. I'll be working on it for the next few days. One advantage is that with the proper cross-package inheritance that R6 allows, it'll be more straightforward for developers to extend ggplot2.

[thomasp85]

If you stumble over a reason for why adding a new geom calling a custom grob function outside of the ggplot2 namespace will fail, please let me know:-)

[wch]

I've made the change in 560ded3.