Skip to content
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

Merged
merged 6 commits into from
Aug 15, 2024

Conversation

catalamarti
Copy link
Contributor

fixes #1510

Copy link
Member

@hadley hadley left a 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 😄

list(key = c("x", "y", "z"), val = 1:3) %>%
tidyr::pivot_wider_spec(spec)
)
# consider removing if we do not want dbplyr in suggests
Copy link
Member

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.

key = c("x", "y", "z")
)

expect_no_error(
Copy link
Member

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)

@@ -1,3 +1,29 @@
test_that("error if input is not a data.frame", {
spec <- tibble::tibble(
Copy link
Member

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).

@DavisVaughan
Copy link
Member

@hadley @catalamarti we actually already have check_data_frame() which could be useful here!

@catalamarti if you use check_data_frame(), I'd encourage you to use it at the top of pivot_wider_spec(), right after the check_dots_empty0(...) call. We typically try and group all the check_ functions together.

You could also pass the error call through, like:

check_data_frame(data, call = error_call)

@DavisVaughan DavisVaughan merged commit 9966c04 into tidyverse:main Aug 15, 2024
12 checks passed
@DavisVaughan
Copy link
Member

Thanks so much @catalamarti!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pivot_wider_spec() should check if data is a data frame
3 participants