Skip to content

Conversation

@ateucher
Copy link
Contributor

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
Contributor 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)

@ateucher
Copy link
Contributor Author

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

@ateucher
Copy link
Contributor Author

ateucher commented Aug 28, 2025

@mpadge I finally found some time to finish this off. Now it fails with an ✖️ if all examples are wrapped in dontrun{}, 👀 if some examples are wrapped in dontrun{} (by setting check_type = "none_watch"), and ✔️ if none are wrapped in don't run.

It looks like the failures here on ubuntu are because the pkgcheck package itself fails the uses_dontrun test (with 👀), so fails in test-extra_checks. It looks like some other tests are overridden. Should I do the same for uses_dontrun?

@ateucher ateucher requested a review from mpadge August 28, 2025 19:13
@mpadge
Copy link
Member

mpadge commented Aug 29, 2025

Thanks @ateucher, great to see this moving again. Can you please do this:

testthat::test_file('tests/testthat/test-extra-checks.R')
testthat::snapshot_review('extra-checks/')

and accept and merge the changes? Thanks

@codecov
Copy link

codecov bot commented Aug 29, 2025

Codecov Report

❌ Patch coverage is 97.02970% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.54%. Comparing base (a7e278d) to head (e5ec60d).
⚠️ Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
R/check-uses-dontrun.R 96.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #246      +/-   ##
==========================================
+ Coverage   92.44%   92.54%   +0.10%     
==========================================
  Files          46       47       +1     
  Lines        3479     3567      +88     
==========================================
+ Hits         3216     3301      +85     
- Misses        263      266       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ateucher
Copy link
Contributor Author

Ok, that's done @mpadge. I also followed the pattern for fns_have_exs in test-extra-checks and set uses_dontrun to pass: cec2b9e

@mpadge
Copy link
Member

mpadge commented Sep 2, 2025

That's awesome, thanks so much @ateucher for the great work. And with due apologies for ongoing requests, but this one is knida important: Can you please add yourself to https://github.com/ropensci-review-tools/pkgcheck/blob/main/DESCRIPTION as "aut"? Then we're good to go; thanks so much!

I really like the conditional use of out$check_type <- "none_watch" too - that's a nice extension!

@ateucher
Copy link
Contributor Author

ateucher commented Sep 2, 2025

Thanks @mpadge!

@mpadge mpadge merged commit df222b4 into ropensci-review-tools:main Sep 3, 2025
8 checks passed
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