Consistent ... handling#655
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
I think it's a fine idea to rename .parent to _parent, and similar.
I'm more ambivalent on splice_dots(), as implemented. It forces promises, and I think the name is misleading.
Also, my experience with reticulate::tuple() is that conveniently unpacking a list is sometimes quite a mental tax in practice, since, when writing against it, you're always guessing or trying to anticipate whether an argument may or may not be unpacked. Eventually, you come to the conclusion that the only reliable way to use it is with tuple(list(...)).
Another pattern I've seen sometimes, and kind of like, is to do something like this:
new_object <- function(.object, ..., .props = list(...)) {}edit: just saw that you've already suggested the same in #497 (comment)
| # `from` is a base or S3 object; pass it as `.data` to the constructor | ||
| return(to(.data = from, ...)) | ||
| user_args$.data <- from | ||
| return(do.call(to, user_args)) |
There was a problem hiding this comment.
This approach now forces ..., which will break any convert methods that want to introspect or delay evaluation of ....
There was a problem hiding this comment.
That's true, but it's not a change: check out the old line 152 below.
(I mean now we do force it for this very specific case, but that's only available in the dev version, so it's not a breaking change).
|
In general, I am also in favour of more explicit unpacking, but this is an idiom that's used elsewhere in base R (e.g. Similarly, in an ideal world, we wouldn't force the dots so early, but in all the existing use cases they were already forced at some point, and I don't think we've ever considered NSE for property setters, and I doubt we'd want to do the work to fully flesh that out. |
|
@lawremi do you have any opinions here? Are there patterns in base/methods that you think we should copy? |
lawremi
left a comment
There was a problem hiding this comment.
This is a nice improvement.
It would have been nice if methods::initialize() behaved like this, instead of treating every unnamed argument as a prototype. new_object() covers the common case with _parent. I don't recall ever passing multiple unnamed arguments to initialize().
|
Reading some more of the discussion, as mentioned, there are examples of both explicit and implicit unpacking in R/Bioc. Since every argument to |
...to be a single unnamed list. Fixes Constructors: Usingrlang::injectwithnew_object()#497._prefix to avoid accidental matches to existing properties. Fixesset_props()fails when setting property namedobject#423.