Skip to content

Conversation

@chinandrew
Copy link
Collaborator

Fixes #270

Summary of changes

  • Update the metadata check in the plotting function to look specifically at mean and stdev values
  • Add test that the warning is raised if one of the values is missing and not raised if both are present.

@chinandrew chinandrew changed the title Update meta check Update R plotting metadata check Nov 24, 2020
@chinandrew chinandrew changed the base branch from main to r-pkg-devel November 24, 2020 20:54
@chinandrew chinandrew removed the request for review from krivard November 24, 2020 20:54
Copy link
Contributor

@capnrefsmmat capnrefsmmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; a couple suggested improvements

Comment on lines 138 to 140
"Metadata for signal mean and standard deviation not",
"available; defaulting to observed mean and standard",
"deviation to set plot range.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a warning class means we don't have to match the message:

Suggested change
"Metadata for signal mean and standard deviation not",
"available; defaulting to observed mean and standard",
"deviation to set plot range.")
class = "covidcast_plot_meta_not_found")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tried this, but couldn't get it to work since this passes (as expected)

  expect_warning(plot(fake_data), class="covidcast_plot_meta_not_found")

but this also passes

  expect_warning(plot(fake_data), class="this_should_fail")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wat

sigh:

> expect_warning(warn("ducks", class="duck_warning"), class="boing")
> expect_error(abort("ducks", class="duck_error"), class="boing")
Error: `abort("ducks", class = "duck_error")` threw an error with unexpected class.
Expected class: boing
Actual class:   duck_error/rlang_error/error/condition
Message:        ducks

So I guess the documentation is a lie for warnings. We might be able to fix this by opting in to testthat's third edition, since that unifies the handling for all three. Worth trying?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work

> library(testthat)
> local_edition(3)
> expect_warning(warn("ducks", class="duck_warning"), class="boing")
Error: `warn("ducks", class = "duck_warning")` did not throw the expected warning.
In addition: Warning message:
ducks 

Updated code to use 3rd ed

chinandrew and others added 2 commits November 24, 2020 13:29
@capnrefsmmat capnrefsmmat merged commit 06ef5fc into r-pkg-devel Nov 25, 2020
@chinandrew chinandrew deleted the update-meta-check branch November 25, 2020 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Signals without metadata fail to plot

4 participants