-
Notifications
You must be signed in to change notification settings - Fork 27
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
Switch from rlang::abort()
in parameters.R
, grids.R
and misc.R
#351
Conversation
rlang::abort()
in parameters.R
, grids.R
and misc.R
.rlang::abort()
in parameters.R
, grids.R
and misc.R
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woop! Bye bye paste0
! 👋
Would you please add tests for the messages that don't have one yet? I spotted an instance where the call has not yet been threaded through and another one with a test missing and I assume there might be more that are still missing a test.
cli::cli_abort( | ||
"{.arg x} must be a data frame to construct a new grid from." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you thread the call through and give this message a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can thread through the call. but this code as far as I can seen isn't possible to reach naturally. It is only applied to the output of functions like make_max_entropy_grid()
and make_latin_hypercube_grid()
which returns
I went in add added the missing tests where i could find them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Having more tests is super helpful.
dials has quite a lot of code paths so I'm not surprised you found another piece of probably dead code. I'll make an issue for it, thank you for threading the call through for now!
Ref: #311
This PR changes
rlang::abort()
tocli::cli_abort()
inparameters.R
,grids.R
andmisc.R
.