Consistent ... handling#655
Conversation
|
For consistency we should do this in |
hadley
changed the title
|
Use |
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).
| # Collect `...` into a named list. As a convenience, a single unnamed list is | ||
| # spliced in so its elements become the values, making it easy to supply | ||
| # values programmatically. All values must be named. | ||
| splice_dots <- function(..., error_call = sys.call(-1)) { |
There was a problem hiding this comment.
I think this needs a better name. Maybe collect_dots()?
| #' with property names supplied in `...`; see [new_property()] for details. | ||
| #' @param ... Name-value pairs of properties to set. As a convenience, you can | ||
| #' supply a single unnamed list instead of individual name-value pairs. | ||
| update_props <- function(`_object`, ...) { |
There was a problem hiding this comment.
Reading through this skill, I can't help but wonder, should set_props() simply take two arguments and we can avoid all this pain by simply removing the potential for name clashes?
set_props <- function(object, props) {} # where props is a named list
|
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? |
2 participants
...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.