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

feat(epi[x]_slide): hint on forgotten syntax specifying comp #533

Merged
merged 11 commits into from
Oct 3, 2024

Conversation

brookslogan
Copy link
Contributor

Checklist

Please:

  • Make sure this PR is against "dev", not "main" (unless this is a release
    PR).
  • [-] Request a review from one of the current main reviewers:
    brookslogan, nmdefries.
  • Makes sure to bump the version number in DESCRIPTION. Always increment
    the patch version number (the third number), unless you are making a
    release PR from dev to main, in which case increment the minor version
    number (the second number).
  • Describe changes made in NEWS.md, making sure breaking changes
    (backwards-incompatible changes to the documented interface) are noted.
    Collect the changes under the next release number (e.g. if you are on
    1.7.2, then write your changes under the 1.8 heading).
  • See DEVELOPMENT.md for more information on the development
    process.

Change explanations for reviewer

Tries to provide some hints when a user has forgotten part of the syntax when specifying their slide computation, e.g., the tilde in a formula. It's just a hint so we don't false-diagnose e.g.

epix_slide(archive, my_failing_forecaster_function_factory(my_forecaster_args))

Perfectly distinguishing between forgotten syntax and the above type of error may allow us to just fix it up, but likely requires significantly more complex slide code, so we could try these hints first.

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

@brookslogan brookslogan requested a review from dshemetov October 1, 2024 00:54
@brookslogan brookslogan force-pushed the lcb/hint-on-nontidy-f-force-errors branch 2 times, most recently from fc256bb to eb997a0 Compare October 1, 2024 00:57
@brookslogan brookslogan marked this pull request as draft October 1, 2024 00:58
@brookslogan brookslogan force-pushed the lcb/hint-on-nontidy-f-force-errors branch from eb997a0 to e5adcbd Compare October 1, 2024 01:16
@brookslogan brookslogan marked this pull request as ready for review October 1, 2024 01:16
DESCRIPTION Outdated Show resolved Hide resolved
Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks! Didn't test it, since the tests look good enough to me 👍

brookslogan and others added 10 commits October 1, 2024 15:23
* Mention what's acceptable for yearmonth time_type
* Mention Inf in validation errors as acceptable iff it's actually acceptable.
* Reject any other strange "int" classes that pass test_int. (It rejects Date
  and POSIXt, but perhaps there are others.)
* Refactor to use helper function test_sensible_int instead of test_int (as
  latter accepts difftimes and makes logic look confusing).
@brookslogan
Copy link
Contributor Author

brookslogan commented Oct 2, 2024

While addressing some lints & testing annoyances, I realized existing error messages may not hint enough for someone who is expecting epi_slide_opt interface. I tweaked things a bit and added a bit of a lazy check for whether they are trying to use .col_names (only detecting whether they pass it by name).

@dshemetov If you have time, could you please take a look? Think it's ready to merge (DESCRIPTION, NEWS.md etc. up to date) if you think it looks good. (This might be new diffs, though apparently I was missing snapshot files originally so it's not that elucidating..)

@brookslogan brookslogan requested a review from dshemetov October 2, 2024 20:49
Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

For some reason seeing diffs from #535 and #532 here, I thought those were already in dev. Strange. Oh well, this looks good to me!

@brookslogan brookslogan merged commit daeb5df into dev Oct 3, 2024
5 checks passed
@brookslogan brookslogan deleted the lcb/hint-on-nontidy-f-force-errors branch October 3, 2024 18:14
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.

Force .f argument usage in epix_slide
2 participants