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

Switch from rlang::abort() in parameters.R, grids.R and misc.R #351

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

EmilHvitfeldt
Copy link
Member

Ref: #311

This PR changes rlang::abort() to cli::cli_abort() in parameters.R, grids.R and misc.R.

@EmilHvitfeldt EmilHvitfeldt changed the title Switch from rlang::abort() in parameters.R, grids.R and misc.R. Switch from rlang::abort() in parameters.R, grids.R and misc.R Sep 19, 2024
Copy link
Member

@hfrick hfrick left a 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.

Comment on lines +256 to +258
cli::cli_abort(
"{.arg x} must be a data frame to construct a new grid from."
)
Copy link
Member

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?

Copy link
Member Author

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

R/parameters.R Show resolved Hide resolved
@EmilHvitfeldt
Copy link
Member Author

I went in add added the missing tests where i could find them

Copy link
Member

@hfrick hfrick left a 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!

@hfrick hfrick merged commit 555493b into main Sep 23, 2024
11 checks passed
@hfrick hfrick deleted the cli-abort-first-half branch September 23, 2024 09: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.

2 participants