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

Using class property seems to clash with object's class #425

Closed
jmbarbone opened this issue Aug 26, 2024 · 2 comments · Fixed by #484
Closed

Using class property seems to clash with object's class #425

jmbarbone opened this issue Aug 26, 2024 · 2 comments · Fixed by #484
Assignees
Milestone

Comments

@jmbarbone
Copy link

library(S7)
packageVersion("S7")
#> [1] '0.1.1.9000'

Foo <- new_class(
  "Foo",
  properties = list(
    class = new_property(class_character, default = "Class"),
    other = new_property(class_character, default = "Other")
  )
)

x <- Foo()

x@other
#> [1] "Other"
x@class # retrieves information about x
#> [1] "Foo"       "S7_object"
class(x)
#> [1] "Foo"       "S7_object"

# sets fine
x@other <- "New"
x@other
#> [1] "New"

# fails
try({
  x@class <- "New"
  x@class
})
#> Error : `object` must be an <S7_object>, not a S3<New>

Created on 2024-08-26 with reprex v2.1.1

@lawremi
Copy link
Collaborator

lawremi commented Aug 29, 2024

If we end up making @() a general attribute getter for any R object, then we would want to reserve the class property name. For now, I'd suggest using a different name.

@t-kalinowski
Copy link
Member

We should check for "reserved" property names and error in new_class(). Besides class, others that I imagine might produce problems: dim, names, dimnames, tsp, levels (for factor)

@t-kalinowski t-kalinowski self-assigned this Oct 28, 2024
@t-kalinowski t-kalinowski added this to the v0.2.0 milestone Oct 28, 2024
t-kalinowski added a commit that referenced this issue Oct 30, 2024
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

Successfully merging a pull request may close this issue.

4 participants
@lawremi @t-kalinowski @jmbarbone and others