Use {cli} for error-messages and handling [SUGGESTION]
Closed this issue · 12 comments
Hi,
I was wondering wether there is any interest in using the {cli}-package for error-messages and handling. In my own package {cryptoQuotes} I use a high-level wrapper to assert the argument values, which returns a detailed and formatted error-message.
The function can be seen below, and I have created a MWE for demonstration.
Function
# assert function
assert <- function(
...,
error_message = NULL) {
# 1) count number of expressions
# in the ellipsis - this
# is the basis for the error-handling
number_expressions <- ...length()
named_expressions <- ...names()
# 2) if there is more than
# one expression the condtions
# will either be stored in an list
# or pased directly into the tryCatch/stopifnot
if (number_expressions != 1 & !is.null(named_expressions)){
# 2.1) store all conditions
# in a list alongside its
# names
conditions <- c(...)
# 2.2) if !is.null(condition_names) the
# above condition never gets evaluated and
# stopped otherwise, if there is errors
#
# The condition is the names(list()), and is
# the error messages written on lhs of the the assert
# function
if (all(conditions)) {
# Stop the funciton
# here if all conditions
# are [TRUE]
return(NULL)
} else {
cli::cli_abort(
message = c(
"x" = named_expressions[which.min(conditions)]
),
call = sys.call(
1 - length(sys.calls())
)
)
}
}
# 3) if there length(...) == 1 then
# above will not run, and stopped if anything
tryCatch(
expr = {
eval.parent(
substitute(
stopifnot(exprs = ...)
)
)
},
error = function(error){
# each error message
# has a message and call
#
# the call will reference the caller
# by default, so we need the second
# topmost caller
cli::cli_abort(
# 3.1) if the length of expressions
# is >1, then then the error message
# is forced to be the internal otherwise
# the assert function will throw the same error-message
# for any error.
message = if (is.null(error_message) || number_expressions != 1)
error$message else
error_message,
call = sys.call(
1 - length(sys.calls())
)
)
}
)
}Demonstration of the assert()-function
The function itself a bit awkward, but it works like a charm (if you ask me 😆). Below is a simple function, foo, which returns the sum but checks whether the argument a is a numeric-value. I think that such a helper-function would be nice to have in the high-level functions as the ic_dataframe()-function.
# some function
foo <- function(
a,
b) {
assert(
is.numeric(a),
error_message = c(
"x" = sprintf(
fmt = "{.arg a} has to be class {.cls numeric}. Got class {.cls %s}.",
class(a)
)
)
)
a + b
}# demonstration
foo(a = "a", b = 2)
Error in `foo()`:
✖ `a` has to be class <numeric>. Got class <character>.
Run `rlang::last_trace()` to see where the error occurred.Created on 2024-08-08 with reprex v2.1.0
Overhead
However, the assert()-function has a huge overhead cost on computation. See the comparison below against a function, bar(), that uses stopifnot() instead,
bar <- function(
a,
b) {
stopifnot(
is.numeric(a)
)
a + b
}Benchmark
microbenchmark::microbenchmark(
try(bar(a = "a", b = 2), silent = TRUE),
try(foo(a = "a", b = 2), silent = TRUE),unit = "seconds"
)Unit: seconds
expr min lq mean median uq max neval cld
try(bar(a = "a", b = 2), silent = TRUE) 0.000044765 0.0000580305 8.494738e-05 0.000086031 0.0001095555 0.000156752 100 a
try(foo(a = "a", b = 2), silent = TRUE) 0.043861503 0.0449099910 4.864483e-02 0.046740493 0.0492079610 0.175779624 100 bSo it should only be considered if you believe your end-user would benefit from it!
No objection to this, especially after seeing that {cli} is nice and lightweight with no non-base R dependencies. No time in near future to implement for me I'm afraid though.
It should be simple to implement - if there is consensus on using it, I could make a PR during the weekend.
What is your release schedule? Could this update alongside the recent fix I made justify a CRAN update?
I have a private repo that depends on {calendar} 😁
Happy to be a concensus of one on this, yes please @serkor1 !
What is your release schedule? Could this update alongside the recent fix I made justify a CRAN update?
Release schedule: when the CRAN team gets back from holiday on 17th August, for sure!
Feel free to add your name to the DESCRIPTION also 🙏
@Robinlovelace is there any reason you are using the methods::is()- instead of the inherits()-function?
@Robinlovelace is there any reason you are using the
methods::is()- instead of theinherits()-function?
methods::is() is what I've seen in other packages, happy to use inherits() if there are advantages of that.
As what I can tell from the files you don't use the package for anything other than verifying classes, so I think we could get rid of the dependency. There is no advantage to it other than reducing dependencies as far as I know!
{methods} is part of base R so no additional dep, right?
This is my understanding, yes. It doesn't have any dependencies (I think). But if you remove it from Imports in the description-file, you'll get the following error:
❯ checking dependencies in R code ... WARNING
'::' or ':::' imports not declared from:
‘methods’
This is what I was referring to when I said "we could get rid of the dependency"!
No objections to getting rid of it. However, it's a 'hypothetical dependency' because most R installation already have {methods}.
Regarding {cli} and messaging, @maelle and I wrote this blog post with some (in our opinion) good recommendations.