From b2d700fc46baa40fb4b56027ff55159d2435bdae Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Thu, 16 May 2024 10:54:50 -0400 Subject: [PATCH] Use linting to detect undesired function usage (#2541) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Maëlle Salmon --- .Rbuildignore | 1 + .github/workflows/lint.yaml | 34 ++++ .lintr.R | 40 ++++ R/build-favicons.R | 5 +- R/build-home-index.R | 2 +- R/build-home-md.R | 2 +- R/deploy-site.R | 7 +- R/package.R | 2 +- R/pkgdown.R | 3 - R/preview.R | 2 +- R/rmarkdown.R | 8 +- R/theme.R | 2 +- R/utils-fs.R | 4 +- R/utils-io.R | 7 +- tests/testthat/assets/-find-assets.html | 241 ++++++++++++++++++++++++ tests/testthat/test-build-articles.R | 12 +- tests/testthat/test-build-home.R | 4 +- tests/testthat/test-build-reference.R | 2 +- tests/testthat/test-check-built.R | 2 +- tests/testthat/test-highlight.R | 4 +- tests/testthat/test-markdown.R | 2 +- tests/testthat/test-navbar.R | 2 +- tests/testthat/test-package.R | 2 +- tests/testthat/test-templates.R | 4 +- 24 files changed, 350 insertions(+), 44 deletions(-) create mode 100644 .github/workflows/lint.yaml create mode 100644 .lintr.R create mode 100644 tests/testthat/assets/-find-assets.html diff --git a/.Rbuildignore b/.Rbuildignore index 36c50bd651..dd8e335613 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -19,3 +19,4 @@ ^fake-index.html$ ^CRAN-SUBMISSION$ ^tools$ +^\.lintr.R$ diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml new file mode 100644 index 0000000000..c3a09d8dc6 --- /dev/null +++ b/.github/workflows/lint.yaml @@ -0,0 +1,34 @@ +# Workflow derived from https://github.com/r-lib/actions/tree/v2/examples +# Need help debugging build failures? Start at https://github.com/r-lib/actions#where-to-find-help +on: + push: + branches: [main, master] + pull_request: + branches: [main, master] + +name: lint + +permissions: read-all + +jobs: + lint: + runs-on: ubuntu-latest + env: + GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }} + steps: + - uses: actions/checkout@v4 + + - uses: r-lib/actions/setup-r@v2 + with: + use-public-rspm: true + + - uses: r-lib/actions/setup-r-dependencies@v2 + with: + extra-packages: any::lintr, local::. + needs: lint + + - name: Lint + run: lintr::lint_package() + shell: Rscript {0} + env: + LINTR_ERROR_ON_LINT: true diff --git a/.lintr.R b/.lintr.R new file mode 100644 index 0000000000..d9b878563b --- /dev/null +++ b/.lintr.R @@ -0,0 +1,40 @@ +linters <- list(lintr::undesirable_function_linter( + fun = c( + # Base messaging + "message" = "use cli::cli_inform()", + "warning" = "use cli::cli_warn()", + "stop" = "use cli::cli_abort()", + # rlang messaging + "inform" = "use cli::cli_inform()", + "warn" = "use cli::cli_warn()", + "abort" = "use cli::cli_abort()", + # older cli + "cli_alert_danger" = "use cli::cli_inform()", + "cli_alert_info" = "use cli::cli_inform()", + "cli_alert_success" = "use cli::cli_inform()", + "cli_alert_warning" = "use cli::cli_inform()", + # fs + "file.path" = "use path()", + "dir" = "use dir_ls()", + "dir.create" = "use dir_create()", + "file.copy" = "use file_copy()", + "file.create" = "use file_create()", + "file.exists" = "use file_exists()", + "file.info" = "use file_info()", + "normalizePath" = "use path_real()", + "unlink" = "use file_delete()", + "basename" = "use path_file()", + "dirname" = "use path_dir()", + # i/o + "readLines" = "use read_lines()", + "writeLines" = "use write_lines()" + ), + symbol_is_undesirable = FALSE +)) + +exclusions <- list( + "R/import-standalone-obj-type.R", + "R/import-standalone-types-check.R", + "vignettes", + "tests/testthat/assets" +) \ No newline at end of file diff --git a/R/build-favicons.R b/R/build-favicons.R index 02ff058b26..8bdca8f5b5 100644 --- a/R/build-favicons.R +++ b/R/build-favicons.R @@ -84,8 +84,7 @@ build_favicons <- function(pkg = ".", overwrite = FALSE) { cli::cli_abort("API request failed.", .internal = TRUE) } - tmp <- tempfile() - on.exit(unlink(tmp)) + tmp <- withr::local_tempdir() result <- httr::RETRY( "GET", result$favicon$package_url, @@ -120,5 +119,5 @@ copy_favicons <- function(pkg = ".") { has_favicons <- function(pkg = ".") { pkg <- as_pkgdown(pkg) - file.exists(path(pkg$src_path, "pkgdown", "favicon")) + unname(file_exists(path(pkg$src_path, "pkgdown", "favicon"))) } diff --git a/R/build-home-index.R b/R/build-home-index.R index 097bed40e1..bb5179e9b6 100644 --- a/R/build-home-index.R +++ b/R/build-home-index.R @@ -64,7 +64,7 @@ data_home_sidebar <- function(pkg = ".", call = caller_env()) { html_path <- path(pkg$src_path, pkg$meta$home$sidebar$html) if (length(html_path)) { - if (!file.exists(html_path)) { + if (!file_exists(html_path)) { rel_html_path <- path_rel(html_path, pkg$src_path) config_abort( pkg, diff --git a/R/build-home-md.R b/R/build-home-md.R index 9f3a76988f..c139de89e8 100644 --- a/R/build-home-md.R +++ b/R/build-home-md.R @@ -38,7 +38,7 @@ render_md <- function(pkg, filename) { cli::cli_inform("Reading {src_path(path_rel(filename, pkg$src_path))}") body <- markdown_body(filename, strip_header = TRUE) - path <- path_ext_set(basename(filename), "html") + path <- path_ext_set(path_file(filename), "html") render_page(pkg, "title-body", data = list( diff --git a/R/deploy-site.R b/R/deploy-site.R index 28aa8627c6..b35e08c43d 100644 --- a/R/deploy-site.R +++ b/R/deploy-site.R @@ -79,7 +79,7 @@ deploy_site_github <- function( ... ) - cli::cli_alert_success("Deploy completed") + cli::cli_inform(c(v = "Deploy completed")) } #' Build and deploy a site locally @@ -199,10 +199,9 @@ git <- function(..., echo_cmd = TRUE, echo = TRUE, error_on_status = TRUE) { callr::run("git", c(...), echo_cmd = echo_cmd, echo = echo, error_on_status = error_on_status) } -construct_commit_message <- function(pkg, commit = ci_commit_sha()) { +construct_commit_message <- function(pkg = ".", commit = ci_commit_sha()) { pkg <- as_pkgdown(pkg) - commit <- sprintf("%s@%s", pkg$version, substr(commit, 1, 7)) - cli::cli_alert_success("Built site for {cli::col_yellow(pkg$package)}: {.emph {cli::col_green(commit)}}") + cli::format_inline("Built site for {pkg$package}@{pkg$version}: {substr(commit, 1, 7)}") } ci_commit_sha <- function() { diff --git a/R/package.R b/R/package.R index 595b047899..73214a06c3 100644 --- a/R/package.R +++ b/R/package.R @@ -311,7 +311,7 @@ package_vignettes <- function(path = ".") { description = desc, depth = dir_depth(file_out) ) - out[order(basename(out$file_out)), ] + out[order(path_file(out$file_out)), ] } find_template_config <- function(package, diff --git a/R/pkgdown.R b/R/pkgdown.R index 49569fba9b..45f4f0000a 100644 --- a/R/pkgdown.R +++ b/R/pkgdown.R @@ -66,9 +66,6 @@ local_pkgdown_site <- function(path = NULL, meta = NULL, clone = FALSE, env = pa } pkg <- as_pkgdown(path, meta) pkg$dst_path <- withr::local_tempdir(.local_envir = env) - - withr::defer(unlink(pkg$dst_path, recursive = TRUE), envir = env) - pkg } diff --git a/R/preview.R b/R/preview.R index 1fc207938a..f5f362624d 100644 --- a/R/preview.R +++ b/R/preview.R @@ -13,7 +13,7 @@ preview_site <- function(pkg = ".", path = ".", preview = NA) { } if (preview) { - cli::cli_alert_info("Previewing site") + cli::cli_inform(c(i = "Previewing site")) utils::browseURL(path(pkg$dst_path, path, "index.html")) } diff --git a/R/rmarkdown.R b/R/rmarkdown.R index c1bc552a99..61c59e92a7 100644 --- a/R/rmarkdown.R +++ b/R/rmarkdown.R @@ -72,10 +72,10 @@ render_rmarkdown <- function(pkg, input, output, ..., seed = NULL, copy_images = # temporarily copy the rendered html into the input path directory and scan # again for additional external resources that may be been included by R code - tempfile_in_input_dir <- file_temp(ext = "html", tmp_dir = path_dir(input_path)) - file_copy(path, tempfile_in_input_dir) - withr::defer(unlink(tempfile_in_input_dir)) - ext_post <- rmarkdown::find_external_resources(tempfile_in_input_dir) + tempfile <- path(path_dir(input_path), "--find-assets.html") + withr::defer(try(file_delete(tempfile))) + file_copy(path, tempfile) + ext_post <- rmarkdown::find_external_resources(tempfile) ext <- rbind(ext_src, ext_post) ext <- ext[!duplicated(ext$path), ] diff --git a/R/theme.R b/R/theme.R index 2a692ec298..cfca3b2311 100644 --- a/R/theme.R +++ b/R/theme.R @@ -27,7 +27,7 @@ build_bslib <- function(pkg = ".") { } data_deps <- function(pkg, depth) { - if (!file.exists(data_deps_path(pkg))) { + if (!file_exists(data_deps_path(pkg))) { cli::cli_abort( "Run {.fn pkgdown::init_site} first.", call = caller_env() diff --git a/R/utils-fs.R b/R/utils-fs.R index bf98cf4bf9..ea1a12bc53 100644 --- a/R/utils-fs.R +++ b/R/utils-fs.R @@ -72,13 +72,11 @@ out_of_date <- function(source, target) { ) } - file.info(source)$mtime > file.info(target)$mtime + file_info(source)$mtime > file_info(target)$mtime } # Path helpers ------------------------------------------------------------ -file.path <- function(...) stop("Use path!") - path_abs <- function(path, start = ".") { is_abs <- is_absolute_path(path) diff --git a/R/utils-io.R b/R/utils-io.R index b40d977c23..97c7dc2f8d 100644 --- a/R/utils-io.R +++ b/R/utils-io.R @@ -7,15 +7,12 @@ read_file <- function(path) { # Inspired by roxygen2 utils-io.R (https://github.com/klutometis/roxygen/) ----------- -readLines <- function(...) stop("Use read_lines!") -writeLines <- function(...) stop("Use write_lines!") - read_lines <- function(path, n = -1L) { - base::readLines(path, n = n, encoding = "UTF-8", warn = FALSE) + base::readLines(path, n = n, encoding = "UTF-8", warn = FALSE) # nolint } write_lines <- function(text, path) { - base::writeLines(enc2utf8(text), path, useBytes = TRUE) + base::writeLines(enc2utf8(text), path, useBytes = TRUE) # nolint } # Other ------------------------------------------------------------------- diff --git a/tests/testthat/assets/-find-assets.html b/tests/testthat/assets/-find-assets.html new file mode 100644 index 0000000000..64573afa7d --- /dev/null +++ b/tests/testthat/assets/-find-assets.html @@ -0,0 +1,241 @@ + + + + + + + + + +R Markdown Vignette with an Image + + + + + + + + +

R Markdown Vignette with an Image

+

Hadley Wickham

+ + + +

Some words, and then an image like this:

+

The pkgdown logo

+ + + + + + + + + + diff --git a/tests/testthat/test-build-articles.R b/tests/testthat/test-build-articles.R index 49f51ff17a..198382568b 100644 --- a/tests/testthat/test-build-articles.R +++ b/tests/testthat/test-build-articles.R @@ -56,7 +56,7 @@ test_that("articles don't include header-attrs.js script", { html <- xml2::read_html(path) js <- xpath_attr(html, ".//body//script", "src") # included for pandoc 2.7.3 - 2.9.2.1 improve accessibility - js <- js[basename(js) != "empty-anchor.js"] + js <- js[path_file(js) != "empty-anchor.js"] expect_equal(js, character()) }) @@ -94,8 +94,8 @@ test_that("html widgets get needed css/js", { css <- xpath_attr(html, ".//body//link", "href") js <- xpath_attr(html, ".//body//script", "src") - expect_true("diffviewer.css" %in% basename(css)) - expect_true("diffviewer.js" %in% basename(js)) + expect_true("diffviewer.css" %in% path_file(css)) + expect_true("diffviewer.js" %in% path_file(js)) }) test_that("can override options with _output.yml", { @@ -170,8 +170,8 @@ test_that("articles in vignettes/articles/ are unnested into articles/", { suppressMessages(path <- build_article("articles/nested", pkg)) expect_equal( - normalizePath(path), - normalizePath(path(pkg$dst_path, "articles", "nested.html")) + path_real(path), + path_real(path(pkg$dst_path, "articles", "nested.html")) ) # Check automatic redirect from articles/articles/foo.html -> articles/foo.html @@ -257,7 +257,7 @@ test_that("check doesn't include getting started vignette", { pkg <- local_pkgdown_site(test_path("assets/articles-resources")) getting_started <- path(pkg$src_path, "vignettes", paste0(pkg$package, ".Rmd")) file_create(getting_started) - withr::defer(unlink(getting_started)) + withr::defer(file_delete(getting_started)) pkg <- local_pkgdown_site(test_path("assets/articles-resources"), meta = " articles: diff --git a/tests/testthat/test-build-home.R b/tests/testthat/test-build-home.R index f56fadc0d0..b824f5e339 100644 --- a/tests/testthat/test-build-home.R +++ b/tests/testthat/test-build-home.R @@ -6,7 +6,7 @@ test_that("intermediate files cleaned up automatically", { pkg <- local_pkgdown_site(test_path("assets/home-index-rmd")) suppressMessages(build_home(pkg)) - expect_setequal(dir(pkg$src_path), c("DESCRIPTION", "index.Rmd")) + expect_setequal(path_file(dir_ls(pkg$src_path)), c("DESCRIPTION", "index.Rmd")) }) test_that("intermediate files cleaned up automatically", { @@ -16,7 +16,7 @@ test_that("intermediate files cleaned up automatically", { suppressMessages(build_home(pkg)) expect_setequal( - dir(pkg$src_path), + path_file(dir_ls(pkg$src_path)), c("NAMESPACE", "DESCRIPTION", "README.md", "README.Rmd") ) }) diff --git a/tests/testthat/test-build-reference.R b/tests/testthat/test-build-reference.R index 0a4c4672f2..24d3769195 100644 --- a/tests/testthat/test-build-reference.R +++ b/tests/testthat/test-build-reference.R @@ -23,7 +23,7 @@ test_that("examples_env sets width", { code: width: 50 ") - dir.create(path(pkg$dst_path, "reference"), recursive = TRUE) + dir_create(path(pkg$dst_path, "reference")) examples_env(pkg) expect_equal(getOption("width"), 50) diff --git a/tests/testthat/test-check-built.R b/tests/testthat/test-check-built.R index 29a6170be5..203ece692e 100644 --- a/tests/testthat/test-check-built.R +++ b/tests/testthat/test-check-built.R @@ -11,7 +11,7 @@ test_that("readme can use images from vignettes", { test_path("assets/articles-images/man/figures/kitten.jpg"), path(pkg$src_path, "vignettes/kitten.jpg") ) - withr::defer(unlink(path(pkg$src_path, "vignettes/kitten.jpg"))) + withr::defer(file_delete(path(pkg$src_path, "vignettes/kitten.jpg"))) suppressMessages(build_home(pkg)) suppressMessages(build_articles(pkg)) diff --git a/tests/testthat/test-highlight.R b/tests/testthat/test-highlight.R index 852ba5866d..2b921f052d 100644 --- a/tests/testthat/test-highlight.R +++ b/tests/testthat/test-highlight.R @@ -1,5 +1,5 @@ test_that("highlight_examples captures dependencies", { - withr::defer(unlink(test_path("Rplot001.png"))) + withr::defer(file_delete(test_path("Rplot001.png"))) dummy_dep <- htmltools::htmlDependency("dummy", "1.0.0", "dummy.js") widget <- htmlwidgets::createWidget("test", list(), dependencies = dummy_dep) @@ -10,7 +10,7 @@ test_that("highlight_examples captures dependencies", { }) test_that("highlight_text & highlight_examples include sourceCode div", { - withr::defer(unlink(test_path("Rplot001.png"))) + withr::defer(file_delete(test_path("Rplot001.png"))) html <- xml2::read_html(highlight_examples("a + a", "x")) expect_equal(xpath_attr(html, "./body/div", "class"), "sourceCode") diff --git a/tests/testthat/test-markdown.R b/tests/testthat/test-markdown.R index 3e46d7f784..af42a8c2bb 100644 --- a/tests/testthat/test-markdown.R +++ b/tests/testthat/test-markdown.R @@ -6,7 +6,7 @@ test_that("handles empty inputs", { expect_equal(markdown_text_block(""), NULL) path <- withr::local_tempfile() - file.create(path) + file_create(path) expect_equal(markdown_body(path), NULL) }) diff --git a/tests/testthat/test-navbar.R b/tests/testthat/test-navbar.R index 65a570fcd2..0f509f432e 100644 --- a/tests/testthat/test-navbar.R +++ b/tests/testthat/test-navbar.R @@ -93,7 +93,7 @@ test_that("data_navbar() can re-order default elements", { left: [github, search] right: [news] ") - file.create(path(pkg$src_path, "NEWS.md")) + file_create(path(pkg$src_path, "NEWS.md")) expect_snapshot(data_navbar(pkg)[c("left", "right")]) }) diff --git a/tests/testthat/test-package.R b/tests/testthat/test-package.R index f386120c07..f7fa88fecb 100644 --- a/tests/testthat/test-package.R +++ b/tests/testthat/test-package.R @@ -49,7 +49,7 @@ test_that("package_vignettes() detects conflicts in final article paths", { test_that("package_vignettes() sorts articles alphabetically by file name", { pkg <- local_pkgdown_site(test_path("assets/articles")) expect_equal( - order(basename(pkg$vignettes$file_out)), + order(path_file(pkg$vignettes$file_out)), seq_len(nrow(pkg$vignettes)) ) }) diff --git a/tests/testthat/test-templates.R b/tests/testthat/test-templates.R index ec79a5d19e..45f87c053d 100644 --- a/tests/testthat/test-templates.R +++ b/tests/testthat/test-templates.R @@ -73,8 +73,8 @@ test_that("find templates in local pkgdown first", { # Expected contents ------------------------------------------------------- test_that("BS5 templates have main + aside", { - names <- dir(path_pkgdown("bs5", "templates"), pattern = "content-") - names <- path_ext_remove(names) + names <- dir_ls(path_pkgdown("BS5", "templates"), regexp = "content-") + names <- path_ext_remove(path_file(names)) names <- gsub("content-", "", names) templates <- lapply(names, read_template_html,