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 condition expectations in prevision of testthat breaking change #3

Closed

Conversation

lionel-
Copy link

@lionel- lionel- commented Jun 16, 2021

We are planning a breaking change to the condition expectations in testthat release (see NEWS item in r-lib/testthat#1401). This PR implements a preventive fix that works with and without the breaking change.

There is no hurry to get this fix on CRAN since we are skipping one released version of testthat before going through with the change.

@DanChaltiel
Copy link
Owner

Hi @lionel-
Thanks for the PR.
Is this a temporary fix or is it a definitive change?
If I understood correctly, expect_warning() will now return the warning itself so multiple piping like in my file will not work right?

crosstable(...) %>%
    expect_warning(class="crosstable_unnamed_anonymous_warning") %>%     
    expect_warning(class="crosstable_unnamed_lambda_warning")

I'm afraid adding braces everywhere will make this rather unreadable:

{ { x=crosstable(..., 
                       .....) } %>%
    expect_warning(class="crosstable_unnamed_anonymous_warning") } %>%     
    expect_warning(class="crosstable_unnamed_lambda_warning")

@lionel-
Copy link
Author

lionel- commented Jun 21, 2021

Is this a temporary fix or is it a definitive change?

This should be a permanent fix.

If I understood correctly, expect_warning() will now return the warning itself so multiple piping like in my file will not work right?

It should work for the same reason that expect_warning(expect_warning(expr)) works. expect_warning() does not inspect the return value but the side effect of warning. To be sure I just checked with dev testthat which features the breaking change.

@lionel- lionel- mentioned this pull request Sep 20, 2021
43 tasks
@hadley
Copy link
Contributor

hadley commented Sep 20, 2021

We're planning on releasing to CRAN soon, so I'd really appreciate it if you could do a release so we avoid breaking a CRAN check for your package 😄

@DanChaltiel
Copy link
Owner

@hadley Sorry, I can unfortunately not work on this package currently, so this will have to wait a bit :-(
I will obviously make some changes in the next release.

@hadley
Copy link
Contributor

hadley commented Sep 23, 2021

Ok, you'll likely get an email from CRAN when this goes out.

@DanChaltiel
Copy link
Owner

Corrected in 713098e.

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.

None yet

3 participants