-
Notifications
You must be signed in to change notification settings - Fork 420
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
improve error message for non data.frame inputs #1567
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.
Thanks for working on this 😄
tests/testthat/test-pivot-wide.R
Outdated
list(key = c("x", "y", "z"), val = 1:3) %>% | ||
tidyr::pivot_wider_spec(spec) | ||
) | ||
# consider removing if we do not want dbplyr in suggests |
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 would remove this because it doesn't really add anything to the test coverage — you've already used a list to check that we're getting an information about what the user actually supplied. In other words, you're now starting to test the behaviour of obj_type_friendly
, which should be out of scope for this test.
tests/testthat/test-pivot-wide.R
Outdated
key = c("x", "y", "z") | ||
) | ||
|
||
expect_no_error( |
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.
Since this test is about erroring when it's not a data frame, I'd suggest dropping this expectation. (And I wouldn't test it elsewhere just because I know that if pivot_wider_spec
stopped working with data frames so much of tidyr would break)
tests/testthat/test-pivot-wide.R
Outdated
@@ -1,3 +1,29 @@ | |||
test_that("error if input is not a data.frame", { | |||
spec <- tibble::tibble( |
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.
No need to use tibble::
here (or tidyr::
below).
@hadley @catalamarti we actually already have @catalamarti if you use You could also pass the error call through, like: check_data_frame(data, call = error_call) |
84fcc53
to
fd36b2a
Compare
Thanks so much @catalamarti! |
fixes #1510