Have `check_package()` return error or TRUE (on valid)

[peterdesmet]

Suggested in camtraptor July 2023 coding sprint

check_package() currently returns the object itself, so it can be used in pipes, but this reads oddly. Rather, check_package() should return TRUE, exactly how frictionless:::check_package() works.

This change will effect the use in other functions. There it should be used as:

check_package(package)
# next steps

Not:

check_package(package) %>% next steps

[PietrH]

Do we also want to move check_package() outside of zzz.R?

[peterdesmet]

Yes

[damianooldoni]

Thanks @PietrH to work on this! 👍 I will handle this PR after merging #223.

[damianooldoni]

Actually frictionless:::check_package() doesn't return TRUE, but nothing. And actually it 's what I expected by our function as well. I am working on this in #247

[PietrH]

I think they should probably return something invisibly, similar to testthat, there is a vignette on custom expectations: https://testthat.r-lib.org/articles/custom-expectation.html?q=expect#expectation-basics

I chose TRUE because this is the behaviour of assertthat, assertions either return an error, or TRUE if all is good. This way they can be used in internal logic.

On this note, we could use the on_failure() helper from assertthat, this is an example from their documentation;

is_odd <- function(x) {
  assert_that(is.numeric(x), length(x) == 1)
  x %% 2 == 1
}
assert_that(is_odd(2))
# Error: is_odd(x = 2) is not TRUE

on_failure(is_odd) <- function(call, env) {
  paste0(deparse(call$x), " is even")
}
assert_that(is_odd(2))
# Error: 2 is even

testthat seems to mostly return the expectation, so expect_length() will return the length when passing, and a message/error if not.

---

So, do we want check_package() to behave more like assertthat or more like testthat?

[damianooldoni]

Hi @PietrH. Behavior of assertthat seems to me ok. I was actually wrong in my previous comment. Or, let's say, not completely correct.

The difference between our function and the frictionless::check_package() was in the last if statement containing an assertion.

Without the final return(TRUE) you wrote, the output of the function was different depending on the evaluation of the if condition. If the condition was not satisfied the function would return nothing, while if the condition was satisfied than the output was TRUE or FALSE depending on the output of the assertion in the if.

So, I added back the return(TRUE) you wrote. Well done!