Skip to content

Add pkgchk_uses_dontrun() as a watch check #246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ateucher
Copy link

This closes #238 when merged.

I thought it made sense for this to be a "watch" check, since there are valid reasons to use \dontrun{} in examples.

I didn't add tests as I couldn't see an obvious pattern of how to do it by looking at existing test. I did run this on several of my local packages.

One thing I'm not sure about is the print method - I don't see it getting printed, only the summary, and wasn't sure how to trigger that (i.e., see the list of functions that use dontrun{}.

Please see my comment on line 37-38 - I was able to extract the dontrun{} sections from the Rd files, but the code is much harder to read. Using grepl() on the character representation is clearer and I can't think of a situation where that search would return a false positive or a false negative.

Running it on pkgcheck gives:

library(pkgcheck)
check <- pkgcheck("~/dev/ateucher/pkgcheck/", goodpractice = FALSE)
summary(check)
#> 
#> ── pkgcheck 0.1.2.133 ──────────────────────────────────────────────────────────
#> 
#> ✔ Package name is available
#> ✔ has a 'codemeta.json' file.
#> ✔ has a 'contributing' file.
#> ✔ uses 'roxygen2'.
#> ✔ 'DESCRIPTION' has a URL field.
#> ✔ 'DESCRIPTION' has a BugReports field.
#> ✔ Package has at least one HTML vignette
#> ✔ All functions have examples.
#> ✔ Package has continuous integration checks.
#> ℹ Package has unusually large number of 20 Imports (> 98% of all packages)
#> ℹ Examples should not use `\dontrun{}` unless really necessary.
#> 
#> ℹ Current status:
#> ✔ This package may be submitted.
#> 

check
#> Loading required namespace: goodpractice
#> 
#> ── pkgcheck 0.1.2.133 ──────────────────────────────────────────────────────────
#> 
#> ✔ Package name is available
#> ✔ has a 'codemeta.json' file.
#> ✔ has a 'contributing' file.
#> ✔ uses 'roxygen2'.
#> ✔ 'DESCRIPTION' has a URL field.
#> ✔ 'DESCRIPTION' has a BugReports field.
#> ✔ Package has at least one HTML vignette
#> ✔ All functions have examples.
#> ✔ Package has continuous integration checks.
#> ℹ Package has unusually large number of 20 Imports (> 98% of all packages)
#> ℹ Examples should not use `\dontrun{}` unless really necessary.
#> 
#> ℹ Current status:
#> ✔ This package may be submitted.
#> 
#> 
#> ── git ──
#> 
#> • HEAD: 361fdd17
#> • Default branch: main
#> • Number of commits: 1141
#> • First commit: 15-05-2020
#> • Number of authors: 3
#> 
#> 
#> ── Package Structure ──
#> 
#> ℹ Package uses the following languages:
#> • R: 100%
#> • YAML: 0%
#> 
#> ℹ Package has
#> • 3 authors.
#> • 3 vignettes.
#> • No internal data
#> • 20 imported packages.
#> • 12 exported functions (median 17 lines of code).
#> • 291 non-exported functions (median 18 lines of code).
#> • 2 parameters per function (median).
#> 
#> ── Package statistics ──
#> 
#>                     measure  value percentile noteworthy
#> 1                   files_R   46.0       94.4           
#> 4           files_vignettes    3.0       89.5           
#> 5               files_tests   12.0       89.5           
#> 6                     loc_R 4077.0       92.6           
#> 7             loc_vignettes  489.0       76.1           
#> 8                 loc_tests  451.0       68.9           
#> 9             num_vignettes    3.0       91.3           
#> 12                  n_fns_r  303.0       93.4           
#> 13         n_fns_r_exported   12.0       51.4           
#> 14     n_fns_r_not_exported  291.0       95.4       TRUE
#> 15         n_fns_per_file_r    3.4       56.7           
#> 16        num_params_per_fn    2.0        8.1           
#> 17             loc_per_fn_r   18.0       54.9           
#> 18         loc_per_fn_r_exp   17.0       40.3           
#> 19     loc_per_fn_r_not_exp   18.0       58.1           
#> 20         rel_whitespace_R   22.0       94.3           
#> 21 rel_whitespace_vignettes   17.4       55.3           
#> 22     rel_whitespace_tests   26.8       72.8           
#> 23      doclines_per_fn_exp   26.5       27.0           
#> 24  doclines_per_fn_not_exp    0.0        0.0       TRUE
#> 25     fn_call_network_size  177.0       86.3
#> 
#> ℹ Package network diagram is at ['/Users/andy/Library/Caches/R/pkgcheck/static/pkgcheck_pkgstats361fdd17.html'].
#> 
#> 
#> ── goodpractice ──
#> 
#> ℹ 'goodpractice' not included with these checks
#> 
#> ── Package Versions ──
#> 
#>   pkgstats: 0.2.0.58
#>   pkgcheck: 0.1.2.133

cli::cli_inform(pkgcheck:::print_check(check, "uses_dontrun"))
#> The following functions have examples that use `\dontrun{}`:
#> 'checks_to_markdown', 'get_default_github_branch', 'get_gh_token',
#> 'get_latest_commit', 'logfile_names', 'pkgcheck_bg', 'pkgcheck',
#> 'print.pkgcheck', 'read_pkg_guide', 'render_md2html',
#> 'use_github_action_pkgcheck'. Consider using `@examplesIf()` to conditionally
#> run examples instead.

Created on 2025-06-21 with reprex v2.1.1

@ateucher ateucher changed the title Add pkgchk_uses_dontrun as a watch check Add pkgchk_uses_dontrun() as a watch check Jun 21, 2025
@mpadge
Copy link
Member

mpadge commented Jun 23, 2025

Thanks @ateucher. Print method will be improved by #247. It's currently off by default unless checks are listed there, but that will make for easier and more sensible default print behaviour. This is also a great test case for #248, because this check should definitely ✖️ if all examples are \dontrun, but generally 👀 otherwise, or possibly even an additional "always-on" ✔️ if all example code is actively run? Anyway, it'll be great to develop those two in parallel.

@ateucher
Copy link
Author

ateucher commented Jun 23, 2025

Ah, that's a good idea to have an ✖️ if all are dontrun\, and 👀 if some are. I guess then I should parse it more precisely so we know if it's all, some, or none

@maelle
Copy link
Contributor

maelle commented Jun 24, 2025

there are valid reasons to use \dontrun{} in examples.

However, wouldn't it be nice if all those were covered by @examplesIf's machinery instead, so one could see at a glance when exactly the example is problematic (on CRAN for instance)? So dontrun would be a bad pattern.

@mpadge
Copy link
Member

mpadge commented Jun 24, 2025

there are valid reasons to use \dontrun{} in examples.

However, wouldn't it be nice if all those were covered by @examplesIf's machinery instead, so one could see at a glance when exactly the example is problematic (on CRAN for instance)? So dontrun would be a bad pattern.

Been attempting exactly that here, and with that demonstrating that sometimes \dontrun{} is simply unavoidable.

@mpadge
Copy link
Member

mpadge commented Jun 25, 2025

@ateucher What's the status here? Tests and checks fail only because you need to run locally and then testthat::snapshot_review("extra-checks"), and increment this to 22:

expect_length (chks, 21L)


parsed_rds <- lapply(
rd_files,
tools::parse_Rd,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be in tryCatch(), because parse_Rd can and does error on un-parseable files.

any(grepl("\\dontrun", rd_char, fixed = TRUE))
},
logical(1)
)
Copy link
Member

@mpadge mpadge Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, interesting, parse_Rd completely strips \dontrun. Made me curious, so I found the cause here. That also shows that the initial .Rd_get_section() internal function is the one that grabs everything. And that then leads to this:

get_Rd_section <- utils::getFromNamespace (".Rd_get_section", "tools")

path <- "/<path>/<to>/<repo>/man"
rd_files <- fs::dir_ls (path, type = "file", regexp = "\\.Rd$")
parsed_rds <- lapply (rd_files, tools::parse_Rd)
rd_example_tags <- lapply (parsed_rds, function (e) {
    ex <- get_Rd_section (e, "examples")
    tags <- vapply (ex, function (exi) attr (exi, "Rd_tag"), character (1L))
    setdiff (tags, "RCODE") # Most tags are "RCODE" so remove those
})

That list can then be examined for \\dontrun, and enables proper counts for each example and overall. And your fn_names code can also be largely removed, and the code here modified to include function names as well, using Rd_get_section which works same as Rd_get_metadata anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I discovered .Rd_get_section() as well, thanks for the snippet!

@ateucher
Copy link
Author

Sorry for the delay @mpadge. I'll get back to this in the next few days!

ateucher added 4 commits July 11, 2025 13:30
* Use tryCatch when parsing Rd files
* Use .Rd_get_section to pull out examples
* Different summary if all vs some examples use dontrun
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dontrun vs examplesIf
3 participants