Check that the f passed to epi[x]_slide takes enough args
#302
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`testthat::expect_no_error` and `testthat::expect_no_warning` are new, experimental functions available in the newest version of `testthat`. The old `testthat::expect_error`, etc, functions also support testing that no errors are raised by setting the `regexp` arg to `NA`. Use these functions so we don't need to require the very newest version of `testthat`.
We only need to make sure errors/warnings are raised in a subset of cases to make sure `f`s are being passed correctly to the check function. There are separate tests for the check function that are more exhaustive.
brookslogan
requested changes
May 8, 2023
brookslogan
approved these changes
May 16, 2023
There was a problem hiding this comment.
I used a suppressWarnings to prevent some warnings from a warning+error case from popping up during test(). Not sure if we want warning+error but it's probably okay either way. (Feel free to adjust this case.)
I also (hopefully) addressed some technicalities dealing with dots forwarding; this could use some review. I'll go ahead and mark this approved so you can go ahead and merge if it looks all right.
nmdefries
commented
May 16, 2023
There was a problem hiding this comment.
@brookslogan Mostly looks good. Thanks for handling dots, I forgot that that was a complication.
One suggestion that I'll add in, and what looks like an issue -- would like you to verify.
nmdefries
commented
May 17, 2023
Co-authored-by: nmdefries <[email protected]>
* Add some explanatory comments * Use `cli_warn` instead of `sprintf` with `ansi_collapse` so length-2 things are formatted "thing 1 and thing 2" not "thing 1, and thing 2" * Fix default-replacement error message when some mandatory args are absorbed by `f`'s dots. (This could give a faulty message due to broadcasting when there was just one arg with a default before the dots, or potentially an R error in other situations if/when there are >2 mandatory args.) * Add regexp's to our tests to test some of this message preparation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we expect
fto take 2 args,xandg.If
fhas...as one of its args, it ideally would take 2 or more args before...; a warning is raised if not. We don't error in this case to add flexibility. A user may want to handle onlyx, likefunction(x, ...) { <do stuff with x only> }if they don't need the group key or ref time value, letting...capture the other args.If
fdoesn't have...as one of its args, it must take 2 or more args in total, and exits with an error if not.Closes #168