Conversation
![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 the package to version 0.1.0, introducing a comprehensive library of S7 properties and a corresponding set of check functions for argument validation. It also refines the internal logic and argument naming for the integer cleaning utilities. Review feedback identified specific edge cases where infinite values could bypass integer validation or cause runtime errors during modulo operations, suggesting more robust finiteness checks.
| if (length(x) != 1L || is.na(x)) { | ||
| cli::cli_abort("{.var {arg_name}} must be a single non-NA number.") | ||
| } |
There was a problem hiding this comment.
The current implementation of check_integer_scalar allows infinite values (e.g., Inf, -Inf) because Inf == round(Inf) is TRUE in R. Infinite values are typically not considered whole numbers. It is safer to explicitly check for finiteness to ensure the input is a valid integer-like scalar.
if (length(x) != 1L || is.na(x) || !is.finite(x)) {
cli::cli_abort("{.var {arg_name}} must be a single finite number.")
}| if (is.integer(x)) { | ||
| return(x) | ||
| } else if (is.numeric(x)) { | ||
| if (all(x %% 1 == 0)) { |
There was a problem hiding this comment.
The check all(x %% 1 == 0) will return NA if any element in x is infinite (since Inf %% 1 is NaN). This will cause the if statement to throw an error: missing value where TRUE/FALSE needed. It is better to explicitly check for finiteness before performing the modulo operation.
if (all(is.finite(x)) && all(x %% 1 == 0)) {There was a problem hiding this comment.
Pull request overview
Release-oriented update that introduces an S7 property library and expands the package’s argument-validation helpers to align S7 property validation with user-facing check_*() functions.
Changes:
- Added a comprehensive set of S7 properties (scalars/vectors + bounded/enum factories) and corresponding documentation.
- Expanded
check_*()with new scalar/vector validators and updated some legacy check signatures/defaults. - Updated cleaning utilities (
clean_int,clean_posint), tests, exports, and release metadata/docs (DESCRIPTION/NEWS/NAMESPACE/man pages).
Reviewed changes
Copilot reviewed 69 out of 70 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
R/check.R |
Adds many new check_*_scalar / vector validators and updates legacy check helpers. |
R/00_S7_properties.R |
Introduces the S7 property library + factories and moves optional() here. |
R/01_S7_generics.R |
Adds the repr S7 generic definition. |
R/clean.R |
Renames xname→arg_name, changes integer coercion and NA handling in cleaning helpers. |
R/types.R |
Removes old location of optional(). |
tests/testthat/test-check.R |
Adds tests for new vector checks and check_pos_integer_scalar(). |
tests/testthat/test-00_S7_properties.R |
Adds construction-time validation tests for each S7 property. |
NAMESPACE |
Exports new S7 properties and new check helpers. |
DESCRIPTION |
Bumps version/date and updates description for S7 property library. |
NEWS.md |
Adds a changelog file (currently entries don’t match the 0.1.0 bump). |
man/*.Rd |
Adds/updates roxygen-generated documentation for new properties/checks and some param renames. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # %% check_character_scalar ---- | ||
| #' Check character scalar | ||
| #' | ||
| #' @param x Character: Value to check. Must be a single non-NA, non-empty string. | ||
| #' @param arg_name Character: Argument name to use in error messages. | ||
| #' | ||
| #' @return Called for side effects. Throws an error if checks fail. | ||
| #' | ||
| #' @author EDG | ||
| #' @export | ||
| #' | ||
| #' @examples | ||
| #' check_character_scalar("hello") | ||
| #' # Throw error: | ||
| #' try(check_character_scalar("")) | ||
| #' try(check_character_scalar(NA_character_)) | ||
| #' try(check_character_scalar(c("a", "b"))) | ||
| check_character_scalar <- function(x, arg_name = deparse(substitute(x))) { | ||
| if (!is.character(x)) { |
There was a problem hiding this comment.
A large set of new exported check_*_scalar / optional-scalar helpers is introduced here, but there are currently no unit tests exercising these new scalar checks (only the vector checks and check_pos_integer_scalar are covered in tests/testthat/test-check.R). Adding focused tests for the scalar variants (valid cases + key failure modes like NA, wrong type, Inf) would prevent regressions and verify error behavior.
| #' non-NULL case. Otherwise, create a new S7 property with appropriate validator using | ||
| #' `S7::new_property()`. | ||
| #' | ||
| #' @param type S7 class. |
There was a problem hiding this comment.
The roxygen docs for optional() say @param type S7 class., but the implementation accepts both S7_base_class and S7_class (and the example passes S7::class_character, a base class). Update the parameter documentation to match the actual accepted types to avoid confusing users and to keep the generated Rd consistent.
| #' @param type S7 class. | |
| #' @param type S7 base class or S7 class. |
|
|
||
|
|
||
| # %% S7-parallel scalar checks ---- | ||
| # Naming mirrors the S7 properties in 00_S7_properties.R one-to-one. |
There was a problem hiding this comment.
This section claims the new check_*_scalar helpers “mirror the S7 properties in 00_S7_properties.R one-to-one”, but there is no check_optional_unit_open_scalar() even though optional_unit_open_scalar exists as a property. Either add the missing optional scalar check (and export/document it) or adjust the comment/list so it’s accurate.
| # Naming mirrors the S7 properties in 00_S7_properties.R one-to-one. | |
| # Naming aligns with the S7 properties in 00_S7_properties.R where matching | |
| # scalar check helpers are implemented. |
| if (!is.numeric(x)) { | ||
| cli::cli_abort("{.var {arg_name}} must be numeric.") | ||
| } | ||
| if (length(x) != 1L || is.na(x)) { | ||
| cli::cli_abort("{.var {arg_name}} must be a single non-NA number.") | ||
| } | ||
| if (x != round(x)) { | ||
| cli::cli_abort("{.var {arg_name}} must be a whole number.") | ||
| } |
There was a problem hiding this comment.
check_integer_scalar() currently treats Inf / -Inf as valid because Inf == round(Inf). For an “integer scalar” check, infinite values should be rejected explicitly (e.g., add an is.finite(x) check after the NA/length check). This also affects check_pos_integer_scalar() which builds on this helper.
| } else if (is.numeric(x)) { | ||
| if (all(x %% 1 == 0)) { | ||
| return(as.integer(x)) | ||
| storage.mode(x) <- "integer" | ||
| return(x) | ||
| } else { | ||
| cli::cli_abort("{.var {xname}} must be integer.") | ||
| cli::cli_abort("{.var {arg_name}} must be integer.") |
There was a problem hiding this comment.
clean_int() uses if (all(x %% 1 == 0)), but when x contains NA, NaN, or Inf, the condition becomes NA and if throws a base error (“missing value where TRUE/FALSE needed”) instead of a cli_abort(). Consider explicitly rejecting non-finite/NA values first (or using isTRUE(all(...))) so failures consistently produce your intended error message.
| ## Version 0.0.4 | ||
|
|
||
| - Added S7 property library | ||
| - Expanded check_ function set | ||
| - Updated rtemis_colors |
There was a problem hiding this comment.
NEWS.md is being introduced for a 0.1.0 release (per DESCRIPTION/PR title), but the top entry is Version 0.0.4 and there is no 0.1.0 section. Add a 0.1.0 heading (and move/rename the current bullets accordingly) so the changelog matches the released package version.
No description provided.