Use := in all tests#658
Conversation
I'm very ambivalent about this. I do like the syntax, but I don't love that there's an obvious way forward to use this more broadly (since it would conflict with existing tidyeval and data.table syntax). Using a custom operator (e.g. `%<-%`) would be easier, but is less compelling because of the way air formats it (multi-line RHS are formatted like `|>`, not `<-`). Reading the tests is a reasonable way to learn about S7 best practices so I really don't like that we're using a syntax that's not widely available. So all that given I think I'm leaning towards dropping `:=` rather than using it more heavily. Fixes #466
|
I'm not sure if the tidyeval semantics have evolved, but at the time this was discussed, it seemed like a single implementation could handle both uses, i.e. they were mutually exclusive. The usage by data.table can be seen as an element of their DSL. |
t-kalinowski
left a comment
There was a problem hiding this comment.
WRT data.table and rlang, that operator is not meant to be invoked as a normal operator, it's handled specially in custom evaluators from the packages. (data.table::`:=` and rlang::`:=` are just stubs that error if called, telling the user something went wrong). I don't think exporting := from S7 will conflict with data.table or rlang.
I agree that having this in tests but not exported is not of much value. We should either decide to export and use everywhere, or drop the idea.
I like the syntax, and don't see a strong reason not to use it. I think we should add it.
|
@lawremi can you break the tie on this? I'm mildly against: I'm worried that @t-kalinowski likes the syntax, thinks it's fine to export it from S7 since it won't cause problems with data.table/tidyeval, it's overall a meaningful improvement in clarity. |
|
Thanks for the opportunity to be the tie breaker on this :) I think the decision here is whether to export and document The question for me has always been whether The question then is whether "name-aware assignment" is a general enough concept to be worth extending the language. Interestingly, as of Python 3.6, in the class declaration context, a field assignment will call class Field:
def __set_name__(self, owner, name):
self.owner = owner
self.name = name
class Person:
age = Field()Taking inspiration from that, `:=` <- function(x, y) {
sym <- substitute(x)
stopifnot(is.symbol(sym))
name <- as.character(sym)
eval.parent(substitute(x <- bind_as(y, name)))
}
bind_as <- function(x, name, ...) UseMethod("bind_as")
bind_as.S7_class_prototype <- function(x, name, ...) {
call <- substitute(x)
call$name <- name
eval.parent(call)
}where the following is added to the top of if (missing(name)) {
return(structure(sys.call(), class = "S7_class_prototype"))
}Using Beyond S7, the applications are numerous, including logger construction, provenance tracking like on model objects (they track their call, why not their name?), and even the model := tar_target({
fit_model(data)
})instead of: tar_target(model, fit_model(data))That might work with the current implementation since the first argument of Actually, it even works now for S7 props: Foo := new_class(properties = list(x = class_numeric))
foo <- Foo(1)
x := prop(foo)
x
# [1] 1but I'm not sure if it's beneficial. Other examples of this potentially helping popular packages: port := config::get() # uses 'value='
width := shiny::reactive(input$width *2) # uses 'label='Maybe we can convince them to add a Beyond name-aware assignment, I couldn't think of any other uses for `:=` <- function(x, y) UseMethod(":=", y)
`:=.S7_class_prototype` <- function(x, y) {
sym <- substitute(x)
stopifnot(is.symbol(sym))
y$name <- as.character(sym)
eval.parent(substitute(x <- y))
}These decisions can be made later though. We can make it as generic as it needs to be, but I wouldn't change it right now, because the argument insertion is so elegant. In conclusion, I think there's pretty strong justification to define |
|
I like it! Just to make sure I understand, this is a little different from Foo := new_class()behave more like: Foo <- new_class(name = "Foo")But with a generic ( That means this form: Foo := new_class()would be closer to: Foo <- bind_as(new_class(), "Foo")rather than: Foo <- new_class(name = "Foo")One minor downside is that this means package authors need to explicitly add support for late naming or renaming. What do you think about also exporting a default E.g., `:=` <- function(x, value) UseMethod(":=", value)
`:=.default` <- function(x, value) {
stopifnot(is.symbol(name <- substitute(x)))
attr(value, "name") <- name <- as.character(name)
assign(name, value, envir = parent.frame())
}
my_env := new.env()
my_env
#> <environment: 0x8909adb98>
#> attr(,"name")
#> [1] "my_env"
my_func := function(a, b) {}
my_func
#> function (a, b)
#> {
#> }
#> attr(,"name")
#> [1] "my_func" |
|
To clarify, I was just exploring some ways that we could make |
|
So we'll use and export |
I'm very ambivalent about this. I do like the syntax, but I don't love that there's an obvious way forward to use this more broadly (since it would conflict with existing tidyeval and data.table syntax). Using a custom operator (e.g.
%<-%) would be easier, but is less compelling because of the way air formats it (multi-line RHS are formatted like|>, not<-). Reading the tests is a reasonable way to learn about S7 best practices so I really don't like that we're using a syntax that's not widely available.So all that given I think I'm leaning towards dropping
:=rather than using it more heavily.Fixes #466