-
Notifications
You must be signed in to change notification settings - Fork 6
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
Closes #393 Extra header line removed #440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something seems amiss here?
There were two sections for |
tests/testthat/test-assertions.R
Outdated
@@ -687,7 +687,6 @@ test_that("assert_numeric_vector Test 39: no error if `arg` is NULL and optional | |||
) | |||
}) | |||
|
|||
# assert_integer_scalar ---- | |||
## Test 40: error if `arg` is not an integer scalar ---- | |||
test_that("assert_integer_scalar Test 40: error if `arg` is not an integer scalar", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems inccorrect - the integer and numeric stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are more issues. For example assert_character_scalar()
is tested in tests 10 and 11 and 17-19. Similar for assert_data_frame()
. Could we clean this up?
And there are some expect_error()
calls which accept any error, e.g.,
expect_error(example_fun2(2))
in test 10. I think we should always specify the class
argument for expect_error()
.
@bundfussr do you want to add those cleanups in this PR or another? If this PR, please feel free to push the updates here. |
I can add them to this PR. |
@bms63 , @ddsjoberg , I have cleaned up Could you double-check that I didn't miss anything or mess up something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks all!!
@ddsjoberg wasn't there a tracking issue for all the error messaging updates? I can't seem to find it |
… 393-assert_int_sclr-uat
@bms63 |
This turns out to just be an extra line added by accident. The checks for
# assert_integer_scalar ----
appear above this chunk.Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.
Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the
main
branch until you have checked off each task.assert_integer_scalar()
test #393 into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)styler::style_file()
to style R and Rmd filesdevtools::document()
so all.Rd
files in theman
folder and theNAMESPACE
file in the project root are updated appropriatelyNEWS.md
under the header# admiral (development version)
if the changes pertain to a user-facing function (i.e. it has an@export
tag) or documentation aimed at users (rather than developers)pkgdown::build_site()
and check that all affected examples are displayed correctly and that all new functions occur on the "Reference" page.lintr::lint_package()
R CMD check
locally and address all errors and warnings -devtools::check()