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

Issue #750 - Introduce a forecast_type argument in validate_forecast() #751

Merged
merged 6 commits into from
Mar 27, 2024

Conversation

nikosbosse
Copy link
Contributor

@nikosbosse nikosbosse commented Mar 26, 2024

Description

As discussed in #750, it would be nice to have a way in validate_forecast() to check the type is exactly what you want.

This PR therefore

  • introduces a forecast_type argument in validate_forecast(), analogously to the one that already exists in as_forecast().
  • creates a new function, assert_forecast_type() that does exactly that, simplifying the code in as_forecast() and minimising repetitive code in validate_forecast(). The rationale for the slightly odd design with actual and desired is that that avoids running get_forecast_type() again in situations where the forecast type is already determined by the class of the object. An earlier version only had a single argument forecast_type() for the desired type and the actual type was determined automatically. This, however, presumes that validation works. I think both versions are fine if you have a preference 🤷
  • creates a new test

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title as follows: issue-number: PR title
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • I have built the package locally and run rebuilt docs using roxygen2.
  • My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@nikosbosse nikosbosse marked this pull request as ready for review March 26, 2024 18:09
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.37%. Comparing base (1a51fa8) to head (232a832).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   95.36%   95.37%   +0.01%     
==========================================
  Files          21       21              
  Lines        1617     1623       +6     
==========================================
+ Hits         1542     1548       +6     
  Misses         75       75              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nikosbosse nikosbosse requested a review from seabbs March 26, 2024 18:09
Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

These cli messages are so pretty.

This, however, presumes that validation works. I think both versions are fine if you have a preference 🤷

Yeah I really have no preference

@seabbs seabbs force-pushed the update-validate_forecast branch from a122de8 to 232a832 Compare March 27, 2024 22:40
@seabbs seabbs enabled auto-merge (rebase) March 27, 2024 22:40
@seabbs seabbs merged commit dca7b8a into main Mar 27, 2024
11 of 12 checks passed
@seabbs seabbs deleted the update-validate_forecast branch March 27, 2024 22:54
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.

2 participants