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

Fix error message formatting for bad_eq_ops #4476

Merged
merged 7 commits into from Jul 9, 2019

Conversation

@rundel
Copy link
Contributor

rundel commented Jul 8, 2019

Addresses #3575

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jul 8, 2019

Could you please add a test that uses expect_known_output() (plus manually printing the condition) so we have a regression test for this? (And so I can see what the new output looks like?)

@rundel

This comment has been minimized.

Copy link
Contributor Author

rundel commented Jul 8, 2019

I've updated the existing test_that checks for this type of error ("filter complains in inputs are named"), they used expect_error() so I've just adopted that approach but can switch over to expect_known_output() if that is preferred.

@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jul 8, 2019

Yeah, I'd prefer expect_known_output() — I think that's a better approach for "testing" that output is useful to a human.

rundel added 2 commits Jul 8, 2019
…n capture_error_msg which is a simplified version of try2 from vctrs.
@rundel

This comment has been minimized.

Copy link
Contributor Author

rundel commented Jul 8, 2019

We've made an attempt at using expect_known_output() for the filter tests. Let us know if this looks good or you have a different structure in mind.

@rundel

This comment has been minimized.

Copy link
Contributor Author

rundel commented Jul 8, 2019

filter(mtcars, x = 1),
"`x` (`x = 1`) must not be named, do you need `==`?",
fixed = TRUE
expect_known_output(

This comment has been minimized.

Copy link
@hadley

hadley Jul 8, 2019

Member

Basic structure looks good, but I think it would be simpler to output all of these to the same file (with a \n between them)

This comment has been minimized.

Copy link
@rundel

rundel Jul 8, 2019

Author Contributor

Makes sense, I hadn't quite grokked expect_known_output() - looking at vctrs more carefully helped me sort things out.

This comment has been minimized.

Copy link
@hadley

hadley Jul 9, 2019

Member

Yeah, it's a bit hokey currently — see more discussion in r-lib/testthat#782

@rundel

This comment has been minimized.

Copy link
Contributor Author

rundel commented Jul 8, 2019

Should be good now

@hadley hadley merged commit ca75e6f into tidyverse:master Jul 9, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hadley

This comment has been minimized.

Copy link
Member

hadley commented Jul 9, 2019

Thanks @rundel and @beatrizmilz!

@rundel rundel deleted the rundel:3575-err_msg_fmt branch Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.