RConsortium / S7

S7: a new OO system for R
https://rconsortium.github.io/S7
Other
399 stars 33 forks source link

Use `:=` everywhere in tests #466

Open hadley opened 1 week ago

hadley commented 1 week ago

i.e. replace all uses of x <- new_class("x", ...) and x <- new_generic("x", ...)

If we're considering exporting it, we might want consider making it work slightly more like the pipe, so the object name is inserted as the first argument on the RHS, rather than always the name argument.

t-kalinowski commented 1 week ago

Setting a name attribute/property seems like it will have broad utility. I'm already finding uses for it in keras3, where many functions take a name argument.

A revised definition:

`:=` <- function(sym, val) {
  cl <- sys.call()
  cl[[1L]] <- quote(`<-`)
  name <- cl[[2L]]

  stopifnot("left hand side must be a symbol" = is.symbol(name))

  if(is.call(cl[[3L]])) {
    cl[[3L]]$name <- as.character(name)
  } else if (S7_inherits(val)) {
    val@name <-  as.character(name)
    cl[[3L]] <- val
  } else {
    attr(val, "name") <- as.character(name)
    cl[[3L]] <- val
  }
  eval.parent(cl)
}
hadley commented 1 week ago

Could we make it a bit more explicit and get rid of the sys.call()?

`:=` <- function(sym, val) {
  lhs <- substitute(sym)
  rhs <- substitute(val)
  stopifnot("left hand side must be a symbol" = is.symbol(lhs))

  cl <- call('<-', lhs, rhs)

  if (is.call(rhs)) {
    cl[[3L]]$name <- as.character(lhs)
  } else if (S7_inherits(val)) {
    val@name <- as.character(lhs)
    cl[[3L]] <- val
  } else {
    attr(val, "name") <- as.character(lhs)
    cl[[3L]] <- val
  }

  eval.parent(cl)
}

I'm not sure what you're thinking with the three branches.

If we're going to use named argument matching rather than position matching, I do think you need to error if the RHS already has a name argument in the call. (But it still seems to me that positional matching is more general, and easier to understand if you already understand the pipe. The obvious extension would be to allow the use of a named _ placeholder if you didn't want it to match the first argument.)

lawremi commented 1 week ago

I like the generality argument. Having more flexibility is always good, as long as it is balanced with convenience. Considering the use cases will be important. The pipe worked out, because the first argument tends to be the subject of the operation. I'm not sure that's true of the name, which might even be optional in many cases, such as in keras3, where it's often forwarded via ....

What are the use cases for the attribute assignment? The smaller the scope, the easier it will be to get this to play nice with other definitions (and maybe eventually get it into base).

t-kalinowski commented 1 week ago

One advantage of having := inject the first positional argument instead of a name argument is that the positional usage will work with R6 (which uses classname) and other frameworks modeled after R6, like keras3::Layer().

It could also be helpful to support usage that allows attaching a name attribute to an object, perhaps accepting () as an escape hatch for modifying an object instead of a call.

Primary usage:

library(S7)
a_generic := new_generic()
a_class := new_class()

Additional usage we may want:

library(R6)
another_class := R6Class()  # classname=

library(keras3)
a_custom_layer_class := Layer()                              # classname=
images := keras_input(shape = c(NA, NA, 3), dtype = "uint8") # name=

some_fn := function(a, b, c) {}  # equivalent to attr(fn, "name") <- "some_fn"
my_state := (new.env())          # equivalent to attr(env, "name") <- "my_state"
t-kalinowski commented 1 week ago

Also, we'll want to add IDE support for := (e.g., named sidebar sections in RStudio)