Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use := everywhere in tests #466

Open
hadley opened this issue Oct 16, 2024 · 5 comments
Open

Use := everywhere in tests #466

hadley opened this issue Oct 16, 2024 · 5 comments

Comments

@hadley
Copy link
Member

hadley commented Oct 16, 2024

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
Copy link
Member

t-kalinowski commented Oct 16, 2024

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
Copy link
Member Author

hadley commented Oct 16, 2024

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
Copy link
Collaborator

lawremi commented Oct 16, 2024

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
Copy link
Member

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants