Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c64c50e058
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| contains_pattern <- function(x, pattern, ignore.case = FALSE, perl = FALSE, fixed = FALSE, useBytes = FALSE) { | ||
| if (!is_string(pattern)) | ||
| return("'pattern' must be a string (length 1 character vector)") |
There was a problem hiding this comment.
Reject missing regex patterns before grepl
When pattern is NA_character_, is_string(pattern) returns TRUE, so this helper proceeds to grepl() and R throws invalid 'pattern' argument instead of the assertion’s normal error formatting. That means callers who pass a missing pattern (e.g., pattern = NA) get a base error rather than the expected assertion message. Consider explicitly rejecting is.na(pattern) (or reusing assert_no_missing for pattern) so missing patterns fail with a consistent assertion error.
Useful? React with 👍 / 👎.
Motivation
assert_all_strings_contain) and a string-level assertion (assert_string_contains) that reuse the same validation logic.Description
contains_pattern()helper that validatespatternand grepl flags and returns whether all elements match the regex.assert_all_strings_contain()as a chain ofassert_character_vector,assert_no_missing, and anassert_create()wrapper aroundcontains_patternwith a default error message.assert_string_contains()asassert_create_chain(assert_string, assert_all_strings_contain)so single-string checks reuse the vector-level logic.assert_matches_regexmanpage and updateNEWS.md,NAMESPACE, and tests to reflect the new assertion names and behavior.Testing
devtools::document()which regenerated Rd files and updatedNAMESPACEsuccessfully.devtools::test()and unit tests passed after adding coverage forassert_string_contains(tests report all tests passing).devtools::check()which completed but returned 1 WARNING (missingqpdffor PDF size checks) and 1 NOTE (unable to verify current time), causing a non-zero exit status; there are no test failures reported.Codex Task