ATFutures / calendar

R interface to iCal (.ics files)
https://atfutures.github.io/calendar/
Other
40 stars 11 forks source link

Use {cli} for error-messages and handling [SUGGESTION] #57

Closed serkor1 closed 1 month ago

serkor1 commented 1 month ago

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 ``` r # 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 :laughing:). 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   b

So it should only be considered if you believe your end-user would benefit from it!

Robinlovelace commented 1 month ago

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.

serkor1 commented 1 month ago

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} 😁

Robinlovelace commented 1 month ago

Happy to be a concensus of one on this, yes please @serkor1 !

Robinlovelace commented 1 month ago

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!

Robinlovelace commented 1 month ago

Feel free to add your name to the DESCRIPTION also :pray:

serkor1 commented 1 month ago

@Robinlovelace is there any reason you are using the methods::is()- instead of the inherits()-function?

Robinlovelace commented 1 month ago

@Robinlovelace is there any reason you are using the methods::is()- instead of the inherits()-function?

methods::is() is what I've seen in other packages, happy to use inherits() if there are advantages of that.

serkor1 commented 1 month ago

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!

Robinlovelace commented 1 month ago

{methods} is part of base R so no additional dep, right?

serkor1 commented 1 month ago

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"!

Robinlovelace commented 1 month ago

No objections to getting rid of it. However, it's a 'hypothetical dependency' because most R installation already have {methods}.

mpadge commented 1 month ago

Regarding {cli} and messaging, @maelle and I wrote this blog post with some (in our opinion) good recommendations.