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

Djm/deprecate #331

Merged
merged 14 commits into from May 13, 2024
Merged

Djm/deprecate #331

merged 14 commits into from May 13, 2024

Conversation

dajmcdon
Copy link
Contributor

@dajmcdon dajmcdon commented May 9, 2024

Checklist

Please:

  • Make sure this PR is against "dev", not "main".
  • Request a review from one of the current epipredict main reviewers:
    dajmcdon.
  • Make sure to bump the version number in DESCRIPTION and NEWS.md.
    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
    0.7.2, then write your changes under the 0.8 heading).

Change explanations for reviewer

In reviewing #296 , some of the error message changes I was suggesting seemed to borrow from existing tidymodels functions. All of these are internal, and we don't actually need them.

@dsweber2 note also the internal versions of purrr functions. This avoids the need to import the package. These are "in" {rlang} and accessible as usethis::use_standalone("r-lib/rlang", "purrr"), for example.

@dajmcdon dajmcdon requested a review from dsweber2 May 9, 2024 20:57
@dajmcdon dajmcdon mentioned this pull request May 9, 2024
9 tasks
R/compat-purrr.R Outdated Show resolved Hide resolved
Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

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

I'm a fan of dropping unnecessary complexity, but I don't understand bake.epi_recipe and epi_juice well enough to be sure we can drop them. Did you run any additional tests that didn't get formalized, and/or which tests do you expect would have thrown a problem if we actually needed bake.epi_recipe and epi_juice?

tests/testthat/test-check_enough_train_data.R Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@dajmcdon
Copy link
Contributor Author

@dsweber2 OK. I refactored bake.epi_recipe() to fix the issue with the tests I'd removed. This now results (correctly) in an epi_df. But note that the column order is different. It is now the order enforced by epiprocess::as_epi_df.

Also added a test to check that my refactor works as expected.

epi_juice() was internal to bake.epi_recipe() and is no longer needed for anything.

@dajmcdon dajmcdon requested a review from dsweber2 May 13, 2024 22:26
Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

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

lgtm. one weird thing I'm getting locally is man/add_model.Rd is getting renamed to man/Add_model.Rd when I run document

@dajmcdon
Copy link
Contributor Author

...man/add_model.Rd is getting renamed to man/Add_model.Rd when I run document

Mine as well. I think it's like that on dev.

@dajmcdon dajmcdon merged commit de6e1db into dev May 13, 2024
2 checks passed
@dajmcdon dajmcdon deleted the djm/deprecate branch May 13, 2024 22:58
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

2 participants