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 cli::cli_abort() in all steps #1262

Merged
merged 6 commits into from
Nov 17, 2023
Merged

Use cli::cli_abort() in all steps #1262

merged 6 commits into from
Nov 17, 2023

Conversation

EmilHvitfeldt
Copy link
Member

Ref: #1237

With this PR, we now have converted all alert() to cli_abort() 🎉

Copy link
Contributor

@simonpcouch simonpcouch left a comment

Choose a reason for hiding this comment

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

Slick! Snazzy! Superb!

tests/testthat/_snaps/cut.md Show resolved Hide resolved
tests/testthat/_snaps/profile.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/range_check.md Outdated Show resolved Hide resolved
tests/testthat/_snaps/relevel.md Outdated Show resolved Hide resolved
@@ -4,23 +4,23 @@
iris_rec %>% step_sample(size = -1)
Condition
Error in `step_sample()`:
! `size` should be a positive number or NULL.
! `size` must be a number larger than or equal to 0 or `NULL`, not the number -1.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
! `size` must be a number larger than or equal to 0 or `NULL`, not the number -1.
! `size` must be `NULL` or a number larger than or equal to 0, not the number -1.

with the current wording, i initially thought NULL completed some part of the clause "a number larger than or equal to 0"

Copy link
Member Author

Choose a reason for hiding this comment

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

This is handled by check_number_decimal() so I'm hesitant to change it

@@ -89,7 +89,7 @@
Condition
Error in `step_window()`:
Caused by error in `prep()`:
! There were 2 term(s) selected but 1 values for the new features were passed to `names`.
! There were 2 terms selected but 1 values for the new features were passed to `names`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
! There were 2 terms selected but 1 values for the new features were passed to `names`.
! There were 2 terms selected but 1 value for the new features was passed to `names`.

pluralization is currently correct for the plural case, though :)

R/time.R Outdated Show resolved Hide resolved
R/time.R Outdated Show resolved Hide resolved
EmilHvitfeldt and others added 3 commits November 17, 2023 08:42
Co-authored-by: Simon P. Couch <[email protected]>
Co-authored-by: Simon P. Couch <[email protected]>
@EmilHvitfeldt EmilHvitfeldt merged commit 4d6338c into main Nov 17, 2023
@EmilHvitfeldt EmilHvitfeldt deleted the cli-final-steps branch November 17, 2023 17:13
Copy link

github-actions bot commented Dec 2, 2023

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 Dec 2, 2023
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