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

Should properties have a default value? #139

Closed
hadley opened this issue Jan 18, 2022 · 8 comments · Fixed by #178
Closed

Should properties have a default value? #139

hadley opened this issue Jan 18, 2022 · 8 comments · Fixed by #178

Comments

@hadley
Copy link
Member

hadley commented Jan 18, 2022

This would be used by the default constructor when the property is not supplied.

If properties can have a default value, should it be supplied automatically? i.e. if the property is an R7 or S4 class, it could call the default constructor. This would have the advantage that you can always easily initialise a class>

@lawremi
Copy link
Collaborator

lawremi commented Jan 21, 2022

This makes sense to me. The methods package initializes slots by just calling new(class), i.e., that is how it generates the default prototype.

@hadley
Copy link
Member Author

hadley commented Jan 26, 2022

This means we'd need to add constructors for non-vector base types (e.g. function), and s3_class() would need to gain a constructor argument. Not sure what to do for unions. Maybe make it an error so the creator has to supply the default? That would probably push us further towards pre-defining some key s3 classes provided by base R. I think this is likely to look something like this:

s3_factor <- s3_class("factor", factor)
s3_data_frame <- s3_class("data.frame", data.frame)

# Needed because S3 functions error when called without arguments
s3_ordered <- s3_class(c("factor", "ordered"), function(x = character(), ...) ordered(x, ...))
s3_date <- s3_class("Date", function(x = integer()) .Date(x))

If your class defines a validator and it fails with the default property values, it'd be your responsibility to either adjust the property defaults or override the constructor.

Should we make the default a function? It seems like it would be convenient to do something like this:

new_property("created_at", s3_date, default = function() Sys.Date())

@lawremi
Copy link
Collaborator

lawremi commented Feb 4, 2022

For unions, the methods package uses the prototype of the first member of the union (where "NULL" is always considered first if present).

In my experience, it's a lot more common to want a constant as the default, so a function might be too complex. It's fairly straightforward to achieve the same effect in the class constructor. That has the minor added benefit of putting all code executed during object instantiation in one place.

@hadley
Copy link
Member Author

hadley commented Feb 4, 2022

Makes sense. What should default = NULL do? Does it make NULL the default, or does it use the default value from the type?

@lawremi
Copy link
Collaborator

lawremi commented Feb 9, 2022

It would be NULL. The automatic default would only apply if no default is provided.

@hadley
Copy link
Member Author

hadley commented Feb 9, 2022

Ah you're thinking the behaviour is conditional on whether or not the argument is missing()? I'm not a big fan of that sort of interface, but maybe it works ok here.

@lawremi
Copy link
Collaborator

lawremi commented Feb 13, 2022

Well the default value of default could be whatever code generates the automatic default, but that wouldn't be NULL.

hadley added a commit that referenced this issue Feb 17, 2022
@hadley
Copy link
Member Author

hadley commented Feb 17, 2022

Somewhat related to this discussion, but in the course of working on this code I realised that the R7 wrapper of the environment base class used new.env(parent = emptyenv()), but this wasn't wrapped in a function so every instance of the class got the same environment.

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

Successfully merging a pull request may close this issue.

2 participants