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

require optional arguments to be named #863

Merged
merged 18 commits into from
Mar 1, 2024
Merged

require optional arguments to be named #863

merged 18 commits into from
Mar 1, 2024

Conversation

simonpcouch
Copy link
Contributor

From NEWS:

Ellipses (...) are now used consistently in the package to require optional arguments to be named. For functions that previously had ellipses at the end of the function signature, they have been moved to follow the last argument without a default value: this applies to augment.tune_results(), collect_predictions.tune_results(), collect_metrics.tune_results(), and the developer-focused estimate_tune_results(), load_pkgs(), and encode_set(). Several other functions that previously did not have ellipses in their signatures gained them: this applies to conf_mat_resampled() and the developer-focused check_workflow(). Optional arguments previously passed by position will now error informatively prompting them to be named. These changes don't apply in cases when the ellipses are currently in use to forward arguments to other functions.

Note that these changes also apply to some other functions that hadn't yet made it to the CRAN version of the package.

Creating as draft as there are many changes here and I'd want to take another look + look into extratests + get started on analogous changes downstream in finetune and agua before calling this ready for review. If you see issues already, though, feel free to chime in.

Copy link
Member

@EmilHvitfeldt EmilHvitfeldt left a comment

Choose a reason for hiding this comment

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

Looks amazing! found 1 place where rlang::check_dots_empty(call = call) wasn't used

R/augment.R Outdated Show resolved Hide resolved
@simonpcouch
Copy link
Contributor Author

Wow, tidysdm is the only revdep breakage, and it's not due to these changes. Right on. :)

@simonpcouch simonpcouch merged commit 884cc7a into main Mar 1, 2024
9 checks passed
@simonpcouch simonpcouch deleted the dots-placement branch March 1, 2024 15:37
simonpcouch added a commit to tidymodels/finetune that referenced this pull request Mar 1, 2024
* address issues from tidymodels/tune#863

* move dots in `collect_*()`

* move dots in `show_best.tune_race()`

note that this adds a `call` argument for compatibility with the `tune_results` method--otherwise, tune will add one when calling `show_best()` inside of `select_best()` and trigger a nonempty dots error

* note change in NEWS

* update Remotes ref

* pass `type` by name

preserves the previous behavior of respecting `type`; this is already tested in extratests

* update tune ref [no ci]
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants