Enable integration of locally-defined checks

[mpadge]

Along the lines of riskmetric which just greps for specific function prefixes in the namespace.

[mpadge]

@dgkf This issue is an important step in bringing our pgkcheck system in line with riskmetric - I'm just using different prefixes, which I think is initially a good idea here (pgkchk_ here for your assess_ functions there). One important question: What is your main motivation for exporting all of your assess_ functions? Is it primarily to be able to export the documentation, so that users can easily see what each test does? Other than that, I see no advantage here of exporting them, and am currently pondering options for other ways to document them, while keeping a minimal namespace for the package (i.e., not exporting). Your insights and experiences from riskmetric would be really helpful here, please 😄

[mpadge]

@dgkf I guess you probably checked all this out at some stage too, but the structure of goodpractice provides a really interesting comparison. All test functions are just appended to a global list of CHECKS, which makes the code very clean and simple - more so, i'd say, than either pkgcheck or riskmetric. One big disadvantage is that checks have internal "description" entries -- like this covr example, or the first entries of all of the rcmdcheck checks -- but these are not at all accessible to users. Nevertheless, I just submitted a PR which would enable direct access to these "description" entries. That would in turn make that whole interface extremely attractive, mostly due to it being so utterly simple. New tests are also easier to add than is currently so in either pgkcheck or riskmetric - the whole procedure only needs one simple example to demonstrate.

The biggest downside I can see is that these new functions are not "first class" functions like the hard-coded internal ones, and can only be explicitly specified as extra_ parameters. Both of our packages (need an ability to) treat additional functions as proper "first class" functions, so can't (easily) follow this method.

More importantly, I want to ensure our packages only become more compatible over time, and not less, so don't want to make any design decisions at this point that are likely to reduce compatibility. Happy to discuss here, or we can wait only 2 days now until riskmetric "sprint" meeting. Maybe one to add to the agenda?

[dgkf]

What is your main motivation for exporting all of your assess_ functions?

My initial thinking was that there may be more administrative users who may be interested in using these underlying functions to automate a validation process or reports. When we started, the tibble interface was sort of a side-note. However, these days it seems most users prefer the tibble workflow.

If you're considering not exporting the pkgchk_ functions, but still want them described somewhere, I think you could try to include a descriptive section in each roxygen header and use @eval and @inheritSection. With these, you can compile a ?pkgcheck overview help page embedding descriptions without exporting each function.

More importantly, I want to ensure our packages only become more compatible over time, and not less

Always a good consideration, and a solid reason to keep the functions non-exported to start.

[mpadge]

Thanks @dgkf - i didn't know about that effect of @eval, which is indeed really useful here!

[mpadge]

This shows how both how to define a check, and the fact that new checks are picked up directly from the global environment. There is a new extra_env parameter which defaults to .GlobalEnv, but can easily be extended:

checks <- pkgcheck(path, extra_env = c (.GlobalEnv, "myotherpkg"))

Here's a reprex of it in action:

library (pkgcheck)
packageVersion ("pkgcheck")
#> [1] '0.0.2'
path <- srr::srr_stats_pkg_skeleton ()

# ----------------------------- NEW CHECKS START
pkgchk_new_check <- function (checks) {
    return (FALSE)
}

output_pkgchk_new_check <- function (checks) {

    out <- list (check_pass = checks$checks$new_check,
                 summary = "",
                 print = "")

    out$summary <- ifelse (out$check_pass,
                           "**NEW CHECK PASSES**",
                           "**NEW CHECK DOES NOT PASS**")

    return (out)
}
# ----------------------------- NEW CHECKS END


x <- capture.output (suppressMessages ({
        roxygen2::roxygenise (path)
        checks <- pkgcheck (path)
}))
summary (checks)
#> 
#> ── demo 0.0.0.9000 ─────────────────────────────────────────────────────────────
#> 
#> ✔ Package name is available
#> ✖ does not have a 'CITATION' file.
#> ✖ does not have a 'codemeta.json' file.
#> ✖ does not have a 'contributing' file.
#> ✔ uses 'roxygen2'.
#> ✖ 'DESCRIPTION' does not have a URL field.
#> ✖ 'DESCRIPTION' does not have a BugReports field.
#> ✖ Package has at no HTML vignettes
#> ✖ These functions do not have examples: [test_fn.Rd].
#> ✖ Continuous integration checks unavailable (no URL in 'DESCRIPTION').
#> ✖ Package coverage is 0% (should be at least 75%).
#> ✖ Statistical standards are missing
#> ✖ This package still has TODO standards and can not be submitted
#> ✖ **NEW CHECK DOES NOT PASS**
#> ✔ R CMD check found no errors.
#> ✔ R CMD check found no warnings.
#> 
#> ℹ Current status:
#> ✖ This package is not ready to be submitted.
#> 

^{Created on 2021-09-30 by the reprex package (v2.0.1.9000)}

Checks need only two functions, both of which must accept the single checks parameter, which has the information needed to implement the check (such as full pkgstats summary and all goodpractice data).

1. The check function itself, which must be prefixed pkgchk_. That can return any arbitrary value which will be then stored in checks$checks$<fn_name>, where fn_name is whatever comes after pkgchk_. In the above case, it is checks$checks$new_check.
2. An output_pkgchk_ function which defines how to convert the check information into summary and/or print information.

[mpadge]

Re-opened because still need to enable new checks to be picked up by print methods

[mpadge]

All done. I won't put reprex here because print output is too long, but code to generate is as simple as this:

#library (pkgcheck)
packageVersion ("pkgcheck")
path <- srr::srr_stats_pkg_skeleton ()

pkgchk_new_check <- function (checks) {
    return (FALSE)
}

output_pkgchk_new_check <- function (checks) {

    out <- list (check_pass = checks$checks$new_check,
                 summary = "",
                 print = "")

    out$summary <- ifelse (out$check_pass,
                           "**NEW CHECK PASSES**",
                           "**NEW CHECK DOES NOT PASS**")
    out$print <- list (message = "New check output",
                       obj = c ("new", "check"))

    return (out)
}

x <- capture.output (suppressMessages ({
        roxygen2::roxygenise (path)
        checks <- pkgcheck (path)
}))
summary (checks)
print (checks)
md <- checks_to_markdown (checks, render = TRUE)

The print method of the output_pkgchk_ function has to have a "message" and "object" component, with the latter currently only implemented for vector objects, but will extend to data.frame-like objects as soon as we have one, and render via knitr::kable.