Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the package’s S7 property helpers so that “optional” properties rely on new_union(NULL, <type>) (and no longer an explicit default = NULL) and tightens nullable handling in factory validators. It also adds specs/ to git/build ignore lists.
Changes:
- Reordered
new_union()arguments for manyoptional_*properties to putNULLfirst and removed explicitdefault = NULL. - Updated
bounded_double_property()andenum()validators to only treatNULLas valid whennullable = TRUE. - Ignored
specs/in.gitignoreand.Rbuildignore.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| R/00_S7_properties.R | Adjusts optional-property union ordering/defaulting and refines nullable validation in factory properties. |
| .gitignore | Ignores specs/ in git. |
| .Rbuildignore | Excludes specs/ from R package builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| optional_character_scalar <- new_property( | ||
| class = new_union(class_character, NULL), | ||
| default = NULL, | ||
| class = new_union(NULL, class_character), | ||
| validator = function(value) { |
There was a problem hiding this comment.
Tests don’t currently assert the new intended behavior that these optional_* properties can be omitted and will default to NULL (now relying on new_union(NULL, ...) rather than an explicit default = NULL). Please add a test that constructs a class with one of these properties and verifies the field is NULL when not provided (and that construction succeeds).
| enum <- function(values, default = NULL, nullable = FALSE) { | ||
| cls <- if (nullable) new_union(class_character, NULL) else class_character | ||
| new_property( | ||
| class = cls, | ||
| validator = function(value) { | ||
| if (is.null(value)) { | ||
| if (nullable && is.null(value)) { | ||
| return(NULL) |
There was a problem hiding this comment.
enum() defaults default = NULL while also defaulting nullable = FALSE. With the updated validator, NULL will now be rejected unless nullable = TRUE, which can make enum(values) produce a property whose default (NULL) is invalid and may break class construction when the field is omitted. Consider either (1) treating default = NULL as “no default” (don’t pass default to new_property() unless non-NULL), (2) automatically setting nullable = TRUE when default is NULL, or (3) choosing a non-NULL default (e.g., first element of values) and documenting it. Add a test covering the chosen behavior.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates optional property definitions in R/00_S7_properties.R by reordering new_union arguments to prioritize NULL as the default value and removes redundant explicit default assignments. It also updates .gitignore and .Rbuildignore to exclude the specs directory. Feedback suggests extending this pattern to bounded_double_property, enum, and the optional() helper function to ensure consistency and prevent potential validation errors where default numeric values might fall outside specified bounds.
| if (nullable && is.null(value)) { | ||
| return(NULL) | ||
| } |
There was a problem hiding this comment.
While updating the validator to check nullable, the class definition for cls (at line 636, currently context) should also be updated to new_union(NULL, class_double) when nullable is TRUE.
Currently, cls is new_union(class_double, NULL), which means the property will default to the default of class_double (likely 0.0 or numeric(0)). If this default value falls outside the specified bounds (e.g., if lower > 0), new_property() will throw an error during construction because the default value fails validation. Swapping the order ensures the default is NULL, consistent with the other optional properties in this PR.
| if (nullable && is.null(value)) { | ||
| return(NULL) | ||
| } |
There was a problem hiding this comment.
For consistency with the other changes in this PR, consider updating the class definition for cls (at line 672, currently context) to new_union(NULL, class_character) when nullable is TRUE. Although enum explicitly sets the default argument in new_property(), using the same union order as other optional properties improves maintainability and follows the pattern established in this PR. Additionally, the optional() helper function at the end of the file (line 717) should likely be updated to S7::new_union(NULL, type) for the same reason.
No description provided.