updated messages to use cli. #1151
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
simonpcouch
approved these changes
Aug 16, 2024
|
|
||
| cli::cli_warn( | ||
| "{.val {min_child_weight}} samples were requested but there were {.val {n}} | ||
| rows in the data. Therefore {.val {n}} will be used" |
There was a problem hiding this comment.
Suggested change
| rows in the data. Therefore {.val {n}} will be used" | |
| rows in the data. Therefore {.val {n}} will be used." |
Comment on lines
+375
to
+378
| cli::cli_abort( | ||
| "The option `counts = TRUE` was used but parameter {.arg nm}, | ||
| was given as signif(x, 3).Please use a value >= 1 | ||
| or set {.code counts = FALSE}." |
There was a problem hiding this comment.
Suggested change
| cli::cli_abort( | |
| "The option `counts = TRUE` was used but parameter {.arg nm}, | |
| was given as signif(x, 3).Please use a value >= 1 | |
| or set {.code counts = FALSE}." | |
| cli::cli_abort( | |
| "The option `counts = TRUE` was used but parameter {.arg nm}, | |
| was given as signif(x, 3). Please use a value >= 1 | |
| or set {.code counts = FALSE}." |
I'm seeing package tests failing related to this test:
Error in `maybe_proportion(x, nm)`: The option `counts = TRUE` was used but parameter `nm`, was given as signif(x, 3).Please use a value >= 1 or set `counts = FALSE`.
Could you transition that test from using expect_error() to expect_snapshot(error = TRUE, ...)?
| cli::cli_warn( | ||
| c( | ||
| "{.val {minCases}} samples were requested but there were {.val {n}} rows in the data.", | ||
| "Therefore {.val {n}} will be used" |
There was a problem hiding this comment.
Suggested change
| "Therefore {.val {n}} will be used" | |
| "{.val {n}} will be used." |
| } | ||
|
|
||
| if (!is.null(early_stop)) { | ||
| if (early_stop <= 1) { | ||
| rlang::abort(paste0("`early_stop` should be on [2, ", nrounds, ").")) | ||
| cli::cli_abort("{.arg early_stop} should be on {.val [2,{nrounds})}") |
There was a problem hiding this comment.
Suggested change
| cli::cli_abort("{.arg early_stop} should be on {.val [2,{nrounds})}") | |
| cli::cli_abort("{.arg early_stop} should be on {.val [2, {nrounds})}.") |
| } else if (early_stop >= nrounds) { | ||
| early_stop <- nrounds - 1 | ||
| rlang::warn(paste0("`early_stop` was reduced to ", early_stop, ".")) | ||
| cli::cli_warn("{.arg early_stop} was reduced to {.val {early_stop}}") |
There was a problem hiding this comment.
Suggested change
| cli::cli_warn("{.arg early_stop} was reduced to {.val {early_stop}}") | |
| cli::cli_warn("{.arg early_stop} was reduced to {.val {early_stop}}.") |
simonpcouch
reviewed
Aug 16, 2024
|
@shum461, do you have interest in working on those suggested changes or would you prefer we take it from here? |
|
Will go ahead and close. Thank you for the PR! |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1139