Merge branch 'main' into MichaelChirico-patch-4

.Rbuildignore

@@ -8,7 +8,7 @@
 ^\.Rproj\.user$
 ^\.idea$
 ^\.dev$
-^\.devcontainers$
+^\.devcontainer$
 ^\.lintr$
 ^\.lintr_new$
 ^wercker\.yml$

.dev/roxygen_test.R

@@ -0,0 +1,54 @@
+# Test to ensure roxygenize() has been run on the current PR
+library(tools)
+library(roxygen2)
+
+old_dir <- file.path(tempdir(), "man")
+if (dir.exists(old_dir)) unlink(old_dir, recursive = TRUE)
+file.copy("man", tempdir(), recursive = TRUE)
+old_files <- list.files(old_dir, pattern = "\\.Rd$")
+new_dir <- "man"
+.Last <- function() unlink(old_dir, recursive = TRUE)
+
+# Rd2txt() prints to its out= argument, so we'd have to compare file contents;
+#   plain parse_Rd() keeps srcref info that encodes the file path, which as.character() strips.
+normalize_rd <- function(rd_file) as.character(parse_Rd(rd_file))
+
+rd_equal <- function(f1, f2) isTRUE(all.equal(normalize_rd(f1), normalize_rd(f2)))
+
+check_roxygenize_idempotent <- function(LOCALE) {
+  Sys.setlocale("LC_COLLATE", LOCALE)
+  roxygenize()
+
+  new_files <- list.files(new_dir, pattern = "\\.Rd$")
+
+  old_not_new <- setdiff(old_files, new_files)
+  if (length(old_not_new) > 0L) {
+    stop("Found saved .Rd files gone from a fresh run of roxygenize(): ", toString(old_not_new))
+  }
+
+  new_not_old <- setdiff(new_files, old_files)
+  if (length(new_not_old) > 0L) {
+    stop("Found new .Rd files from a fresh run of roxygenize(): ", toString(new_not_old))
+  }
+
+  for (file in new_files) {
+    old_file <- file.path(old_dir, file)
+    new_file <- file.path(new_dir, file)
+    if (rd_equal(old_file, new_file)) {
+      next
+    }
+    cat(sprintf("roxygenize() output differs from saved output for %s.\n", file))
+    cat("Here's the 'diff' comparison of the two files:\n")
+    cat("  [---]: saved output in man/ directory\n")
+    cat("  [+++]: roxygenize() output of R/ sources\n")
+    system2("diff", c("--unified", old_file, new_file))
+    stop("Failed in LOCALE=", LOCALE, ".", call. = FALSE)
+  }
+}
+
+# Run the check in a few locales to ensure there's no idempotency issues w.r.t. sorting, too
+for (LOCALE in c("C", "en_US", "hu_HU", "ja_JP")) {
+  check_roxygenize_idempotent(LOCALE)
+}
+
+unlink(old_dir, recursive = TRUE)

.devcontainer/Dockerfile

@@ -0,0 +1,14 @@
+FROM rocker/r-base
+
+RUN apt-get -qq update && \
+  apt-get install -y --no-install-recommends git libxml2-dev
+
+COPY DESCRIPTION .
+
+RUN Rscript -e '                           \
+  install.packages("remotes");             \
+  remotes::install_deps(dependencies = c(  \
+    "Imports",                             \
+    "Config/needs/development"             \
+  ))                                       \
+'

.devcontainer/devcontainer.json

@@ -0,0 +1,3 @@
+{
+    "build": { "dockerfile": "Dockerfile", "context": ".."}
+}

.github/workflows/ensure-metadata-tests.yaml → .github/workflows/repo-meta-tests.yaml

@@ -1,14 +1,14 @@
-# Ensure lint metadata is tested
+# Various repo-level tests for code quality
 on:
   push:
     branches: [main]
   pull_request:
     branches: [main]
 
-name: ensure-metadata-tests
+name: repo-meta-tests
 
 jobs:
-  ensure-metadata-tests:
+  repo-meta-tests:
     runs-on: ubuntu-latest
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
@@ -22,9 +22,17 @@ jobs:
           use-public-rspm: true
 
       - uses: r-lib/actions/setup-r-dependencies@v2
+        with:
+          extra-packages: |
+            any::roxygen2
 
       - name: Ensure lint metadata is tested
         run: |
           options(crayon.enabled = TRUE)
           callr::rscript(".dev/lint_metadata_test.R")
         shell: Rscript {0}
+
+      - name: Ensure roxygen content matches man directory
+        run: |
+          callr::rscript(".dev/roxygen_test.R")
+        shell: Rscript {0}

.lintr

@@ -5,12 +5,9 @@ linters: all_linters(
     todo_comment_linter(
       except_regex = rex::rex(
         "TODO(",
-        group(or(
-          # GitHub issue number #1234, possibly from another repo org/repo#5678
-          list(maybe(one_or_more(alnum, "-"), "/", one_or_more(alnum, ".", "-", "_")), "#", one_or_more(digit)),
-          # GitHub user. TODO(#2450): remove this temporary immunity
-          one_or_more(alnum, "-")
-        )),
+        # GitHub issue number #1234, possibly from another repo org/repo#5678
+        maybe(one_or_more(character_class("a-zA-Z0-9-")), "/", one_or_more(character_class("a-zA-Z0-9._-"))),
+        "#", one_or_more(digit),
         ")"
       )
     ),

DESCRIPTION

@@ -52,6 +52,7 @@ Enhances:
 VignetteBuilder: 
     knitr
 Config/Needs/website: tidyverse/tidytemplate
+Config/Needs/development: pkgload, cli, testthat, patrick
 Config/testthat/edition: 3
 Encoding: UTF-8
 Roxygen: list(markdown = TRUE)

NEWS.md

@@ -22,7 +22,7 @@
 
 ## Changes to default linters
 
-* New default linter `return_linter()` for the style guide rule that terminal returns should be left implicit (#1100, #2354, and #2356, @MEO265 and @MichaelChirico).
+* New default linter `return_linter()` for the style guide rule that terminal returns should be left implicit (#1100, #2343, #2354, and #2356, @MEO265 and @MichaelChirico).
 
 ## New and improved features
 
@@ -62,7 +62,7 @@
 * `print_linter()` for discouraging usage of `print()` on string literals like `print("Reached here")` or `print(paste("Found", nrow(DF), "rows."))` (#1894, @MichaelChirico).
 * `unnecessary_nesting_linter()` for discouraging overly-nested code where an early return or eliminated sub-expression (inside '{') is preferable (#2317 and part of #884, @MichaelChirico).
 * `consecutive_mutate_linter()` for encouraging consecutive calls to `dplyr::mutate()` to be combined (part of #884, @MichaelChirico).
-* `if_switch_linter()` for encouraging `switch()` over repeated `if`/`else` tests (part of #884, @MichaelChirico).
+* `if_switch_linter()` for encouraging `switch()` over repeated `if`/`else` tests (#2322 and part of #884, @MichaelChirico).
 * `nested_pipe_linter()` for discouraging pipes within pipes, e.g. `df1 %>% inner_join(df2 %>% select(a, b))` (part of #884, @MichaelChirico).
 * `nrow_subset_linter()` for discouraging usage like `nrow(subset(x, conditions))` in favor of something like `with(x, sum(conditions))` which doesn't require a full subset of `x` (#2313, #2314 and part of #884, @MichaelChirico).
 * `pipe_return_linter()` for discouraging usage of `return()` inside a {magrittr} pipeline (part of #884, @MichaelChirico).

R/boolean_arithmetic_linter.R

@@ -63,7 +63,7 @@ boolean_arithmetic_linter <- function() {
     xml_nodes_to_lints(
       any_expr,
       source_expression = source_expression,
-      # TODO(michaelchirico): customize this?
+      # TODO(#2464): customize this?
       lint_message = paste(
         "Use any() to express logical aggregations.",
         "For example, replace length(which(x == y)) == 0 with !any(x == y)."

R/expect_length_linter.R

@@ -21,7 +21,7 @@
 #' @seealso [linters] for a complete list of linters available in lintr.
 #' @export
 expect_length_linter <- function() {
-  # TODO(michaelchirico): also catch expect_true(length(x) == 1)
+  # TODO(#2465): also catch expect_true(length(x) == 1)
   xpath <- sprintf("
   parent::expr
     /following-sibling::expr[