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

R6 class should inherit from environment #91

Closed
richierocks opened this issue Aug 17, 2016 · 3 comments

Comments

@richierocks
Copy link
Contributor

commented Aug 17, 2016

Here's the first example from the R6 vignette:

Person <- R6Class("Person",
  public = list(
    name = NA,
    hair = NA,
    initialize = function(name, hair) {
      if (!missing(name)) self$name <- name
      if (!missing(hair)) self$hair <- hair
      self$greet()
    },
    set_hair = function(val) {
      self$hair <- val
    },
    greet = function() {
      cat(paste0("Hello, my name is ", self$name, ".\n"))
    }
  )
)
ann <- Person$new("Ann", "black")

ann is an environment, but its class doesn't inherit from environment.

is.environment(ann)
## [1] TRUE
class(ann)
## [1] "Person" "R6"

This means that S3 methods that work on environments don't work when you call the generic function.

as.list(ann)
## Error in as.vector(x, "list") : 
##   cannot coerce type 'environment' to vector of type 'list'
traceback()
## 2: as.list.default(ann)
## 1: as.list(ann)

as.list.environment(ann) works as expected, but these specific methods are often internal to packages and not available.

It would be useful if R6 objects had class c("R6", "environment").

@gaborcsardi

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

Personally, I am not sure. The environment is the implementation, but if you want an opaque type, then you don't want to inherit from it. What if in the future R6 will have a different implementation? E.g. a couple of environments instead of one? (Which it is, if you consider private members.)

Also, non-portable classes are different, there the environment includes self AFAIR.

@richierocks

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2016

Fair enough. I suppose I'd settle for having an as.list.R6 method that simply called as.list.environment.

@wch

This comment has been minimized.

Copy link
Member

commented Aug 18, 2016

I've thought about this before and I've been on the fence whether to add environment as a class. But I think the idea of having as.list.R6 makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.