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

Use stricter check to bypass validation of x[] #897

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Conversation

Bisaloo
Copy link
Member

@Bisaloo Bisaloo commented Aug 27, 2024

This ensures that object manipulated by [ via dplyr function are still validated. See the report in

#884 (comment).

library(scoringutils)
#> Note: scoringutils is currently undergoing major development changes (with an update planned for the first quarter of 2024). We would very much appreciate your opinions and feedback on what should be included in this major update: https://github.com/epiforecasts/scoringutils/discussions/333

f <- as_forecast_quantile(example_quantile)
#> ℹ Some rows containing NA values may be removed. This is fine if not
#>   unexpected.
f <- f |> 
  dplyr::select(-observed)
#> Warning: ! Error in validating forecast object: Error : Assertion on 'data' failed:
#>   Column 'observed' not found in data.

Created on 2024-08-27 with reprex v2.1.1

I could add tests for this but it would mean adding dplyr as Suggests.

This ensures that object manipulated by [ via dplyr function are still validated.
See the report in #884 (comment).
@seabbs
Copy link
Contributor

seabbs commented Aug 27, 2024

I could add tests for this but it would mean adding dplyr as

What about adding to the testthat config and only running the tests if dplyr is installed (i.e only on our CI). Might be a bit of a hack

@nikosbosse
Copy link
Contributor

Sweet, thanks a lot!

What about adding to the testthat config and only running the tests if dplyr is installed (i.e only on our CI). Might be a bit of a hack

I like this

@Bisaloo
Copy link
Member Author

Bisaloo commented Aug 27, 2024

What about adding to the testthat config and only running the tests if dplyr is installed (i.e only on our CI). Might be a bit of a hack

From what I remember, this doesn't solve the issue since R CMD check sees library(dplyr) or dplyr:: in the tests (even if guarded with skip_if_not_installed()) and gives a warning if it's not listed as a dependency.

Copy link
Contributor

@nikosbosse nikosbosse left a comment

Choose a reason for hiding this comment

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

Thank so much, @Bisaloo !

@nikosbosse nikosbosse merged commit 295b1ea into main Sep 9, 2024
9 checks passed
@nikosbosse nikosbosse deleted the dplyr-compatibility branch September 9, 2024 07:19
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.

3 participants