-
Notifications
You must be signed in to change notification settings - Fork 317
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
Implement expect_without_warnings() and friends #1690
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.
I'm having doubts about this. It looks like we're encouraging testing strings generated in other packages. Should we encourage snapshots instead?
NAMESPACE
Outdated
@@ -124,6 +124,10 @@ export(expect_type) | |||
export(expect_vector) | |||
export(expect_visible) | |||
export(expect_warning) | |||
export(expect_without_condition) |
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 these names are a bit too long. They will dominate the line width, making it harder to read the actual logic.
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.
Do you have any ideas for alternatives? I don't particularly like expect_no_warnings()
because expect_no_warnings(foo(), class = "abc")
doesn't make as much sense to me.
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.
Might be too literary but:
expect_sans_warnings(foo(), class = "foo")
?
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.
Or too French 😛
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.
expect_no_warnings(foo(), class = "abc")
doesn't make as much sense to me.
I don't love the "no" particle but I like the concision.
Does it make sense to read it as "expect no warnings of class 'abc'"?
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.
Ok, you've convinced me
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'm happy with this. I made a few inline proposals and comments. I also test drove it in a couple of packages where I had the sort of DIY test helper that this is meant to replace and all seems well (not a big surprise).
Co-authored-by: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com> Co-authored-by: Lionel Henry <lionel.hry@gmail.com>
I marked these as experimental, which makes me feel better about including them in a patch release. |
Fixes #1679
I've kept the interface deliberately very minimum; let me know if you think there's anything I'm missing.