From 58c6168ba2aabc9f609190ceee41ae5bb560f161 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 22:10:47 +0000 Subject: [PATCH 1/3] Clear the lms issues deferred from the move (#32, #34, #35) psc#33 moved lms here unchanged, so its review findings were deferred. - #35: split R/linters.R into one file per function, as psc's coding-practices chapter asks. The four functions move verbatim; only the man/ source headers change. - #34: condition_class_linter() counted only a named class = argument, so expect_error(f(), "msg", "my_class") was flagged. A new match_expectation_args() matches object, regexp and class as R does: named (exact or unique partial) first, then unnamed in order. Four of the five new tests fail on the old linter. Bump lms to 0.1.2 for the rule change. - #32: re-break lms/README.md at sentence and clause boundaries. It also told readers to install from the lms-v0.1.1 tag, which does not exist, so the examples now pin a commit, as the lab manual does. Closes #32, closes #34, closes #35. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Jt6yVGbqCGUgWio5QUBDex --- lms/DESCRIPTION | 2 +- lms/R/condition_class_linter.R | 23 +-- lms/R/default_linters.R | 33 ++++ lms/R/function_length_linter.R | 45 +++++ lms/R/linters.R | 154 ------------------ lms/R/match_expectation_args.R | 31 ++++ lms/R/snake_case_acros_regex.R | 26 +++ lms/R/undesirable_functions.R | 47 ++++++ lms/README.md | 111 ++++++++----- lms/man/default_linters.Rd | 2 +- lms/man/function_length_linter.Rd | 2 +- lms/man/undesirable_functions.Rd | 2 +- .../testthat/test-condition_class_linter.R | 41 +++++ 13 files changed, 304 insertions(+), 215 deletions(-) create mode 100644 lms/R/default_linters.R create mode 100644 lms/R/function_length_linter.R delete mode 100644 lms/R/linters.R create mode 100644 lms/R/match_expectation_args.R create mode 100644 lms/R/snake_case_acros_regex.R create mode 100644 lms/R/undesirable_functions.R diff --git a/lms/DESCRIPTION b/lms/DESCRIPTION index 3bc9f817..6a840ae0 100644 --- a/lms/DESCRIPTION +++ b/lms/DESCRIPTION @@ -1,6 +1,6 @@ Package: lms Title: Lab Manual Style (UCD-SERG lintr Configuration) -Version: 0.1.1 +Version: 0.1.2 Authors@R: person("UCD-SERG", role = c("aut", "cre"), email = "serg@ucdavis.edu") Description: Shared lintr linter configuration implementing the UCD-SERG diff --git a/lms/R/condition_class_linter.R b/lms/R/condition_class_linter.R index c3c8d737..d0c27775 100644 --- a/lms/R/condition_class_linter.R +++ b/lms/R/condition_class_linter.R @@ -1,8 +1,10 @@ #' Condition-class linter for testthat expectations #' #' Flags `expect_error()`, `expect_warning()`, and `expect_message()` calls -#' that assert on message text (`regexp =` or positional `regexp`) without -#' also asserting on a condition `class =`. +#' that assert on message text (`regexp`) without +#' also asserting on a condition `class`. +#' Either argument may be named, partially named, or positional; +#' `match_expectation_args()` matches them as R does. #' #' Matching only message text is brittle: #' wording can change while the underlying condition type stays correct. @@ -34,21 +36,8 @@ condition_class_linter <- function() { } needs_class <- vapply(calls, function(call) { - arg_names <- xml2::xml_text(xml2::xml_find_all(call, "./SYMBOL_SUB")) - has_class <- "class" %in% arg_names - has_named_regexp <- "regexp" %in% arg_names - - # The second positional argument is the third direct `expr` child: - # function call, first argument, then unnamed `regexp`. - # Requiring an immediate preceding comma excludes named arguments, - # whose value expression is preceded by `EQ_SUB` instead. - positional_regexp <- xml2::xml_find_first( - call, - "./expr[3][preceding-sibling::*[1][self::OP-COMMA]]" - ) - has_positional_regexp <- !inherits(positional_regexp, "xml_missing") - - !has_class && (has_named_regexp || has_positional_regexp) + supplied <- match_expectation_args(call) + "regexp" %in% supplied && !("class" %in% supplied) }, logical(1)) lintr::xml_nodes_to_lints( diff --git a/lms/R/default_linters.R b/lms/R/default_linters.R new file mode 100644 index 00000000..7be13e8c --- /dev/null +++ b/lms/R/default_linters.R @@ -0,0 +1,33 @@ +#' Default UCD-SERG lintr linters +#' +#' The canonical linter set for UCD-SERG repositories, matching the style +#' guide in the lab manual. Applied repositories consume this from a thin +#' `.lintr.R`: +#' +#' ```r +#' linters <- lms::default_linters() +#' exclusions <- list(...) # repo-specific, stays local +#' ``` +#' +#' @return A list of linters, as produced by +#' [lintr::linters_with_defaults()]. +#' @export +default_linters <- function() { + lintr::linters_with_defaults( + return_linter = NULL, + trailing_whitespace_linter = NULL, + lintr::redundant_equals_linter(), + lintr::pipe_consistency_linter(pipe = "|>"), + lintr::object_name_linter( + regexes = c(snake_case_ACROs1 = snake_case_acros_regex()) + ), + lintr::undesirable_function_linter( + fun = undesirable_functions(), + symbol_is_undesirable = TRUE + ), + lintr::cyclocomp_linter(), + function_length_linter(), + function_location_linter(), + condition_class_linter() + ) +} diff --git a/lms/R/function_length_linter.R b/lms/R/function_length_linter.R new file mode 100644 index 00000000..9c65f7d0 --- /dev/null +++ b/lms/R/function_length_linter.R @@ -0,0 +1,45 @@ +#' Function-length linter +#' +#' Flags function definitions (including `\(...)` lambdas) that span more +#' lines than `length_limit`, matching the provisional heuristic documented +#' in "Function Length Limits" in *Principles of Scientific Computing* +#' () +#' ("use `<150` lines... as a trigger to reassess decomposition, not a hard +#' constraint"). lintr has no built-in linter for this +#' (see ); `cyclocomp_linter()` +#' covers the complementary branching-complexity heuristic. +#' +#' @param length_limit Maximum number of lines a function definition may +#' span before being flagged. Defaults to 150, matching the documented +#' heuristic. +#' @return A [lintr::Linter()] object. +#' @export +function_length_linter <- function(length_limit = 150L) { + xpath <- "//FUNCTION/parent::expr | //OP-LAMBDA/parent::expr" + + lintr::Linter(linter_level = "expression", function(source_expression) { + if (!lintr::is_lint_level(source_expression, "expression")) { + return(list()) + } + + xml <- source_expression$xml_parsed_content + fun_defs <- xml2::xml_find_all(xml, xpath) + + n_lines <- as.integer(xml2::xml_attr(fun_defs, "line2")) - + as.integer(xml2::xml_attr(fun_defs, "line1")) + 1L + + lintr::xml_nodes_to_lints( + fun_defs[n_lines > length_limit], + source_expression = source_expression, + lint_message = sprintf( + paste0( + "Function spans more than %d lines; consider decomposing it (see ", + "https://morrison-lab.github.io/psc/chapters/", + "coding-practices.html#sec-function-length-limits)." + ), + length_limit + ), + type = "warning" + ) + }) +} diff --git a/lms/R/linters.R b/lms/R/linters.R deleted file mode 100644 index a19ff331..00000000 --- a/lms/R/linters.R +++ /dev/null @@ -1,154 +0,0 @@ -#' Undesirable functions for UCD-SERG repositories -#' -#' The default lintr undesirable-function map, extended with the UCD-SERG -#' conventions: base and rlang messaging functions are steered toward their -#' `cli::` equivalents, `library()` is discouraged inside package code, and a -#' few of lintr's defaults (`structure()`, `browser()`) are dropped. -#' -#' Exported so a downstream repository can further `lintr::modify_defaults()` -#' it if a single repo needs to diverge, without copy-pasting the whole map. -#' -#' @return A named character vector suitable for -#' [lintr::undesirable_function_linter()]'s `fun` argument. -#' @export -undesirable_functions <- function() { - lintr::modify_defaults( - defaults = lintr::default_undesirable_functions, - - # following https://github.com/r-lib/devtools/blob/2aa51ef/.lintr.R: - # 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()", - - library = paste( - "\nuse `::`, `usethis::use_import_from()`, or `withr::local_package()`", - "instead of modifying the global search path.", - "\nSee:\n", - " and\n", - "", # nolint: line_length_linter. - "\nfor more details." - ), - - structure = NULL, - browser = NULL - # see https://github.com/r-lib/lintr/pull/2227 and - # rebuttal https://github.com/r-lib/lintr/pull/2227#issuecomment-1800302675 - ) -} - -#' snake_case object-name regex (uppercase acronyms allowed) -#' -#' Builds the regex used by the object-name linter so that names like -#' `snake_case_ACROs1` (uppercase acronyms, optional trailing digits) are -#' accepted. See for background. -#' -#' Constructed with `rex::rex()`, which evaluates its arguments in an -#' environment that registers the rex shortcuts (`start`, `upper`, `digit`, -#' `%or%`, ...), so the package does **not** need to attach rex. -#' -#' @return A single regular-expression string. -#' @noRd -snake_case_acros_regex <- function() { - rex::rex( - start, - maybe("."), - list(some_of(upper), maybe("s"), zero_or_more(digit)) %or% - list(some_of(lower), zero_or_more(digit)), - zero_or_more( - "_", - list(some_of(upper), maybe("s"), zero_or_more(digit)) %or% - list(some_of(lower), zero_or_more(digit)) - ), - end - ) -} - -#' Function-length linter -#' -#' Flags function definitions (including `\(...)` lambdas) that span more -#' lines than `length_limit`, matching the provisional heuristic documented -#' in "Function Length Limits" in *Principles of Scientific Computing* -#' () -#' ("use `<150` lines... as a trigger to reassess decomposition, not a hard -#' constraint"). lintr has no built-in linter for this -#' (see ); `cyclocomp_linter()` -#' covers the complementary branching-complexity heuristic. -#' -#' @param length_limit Maximum number of lines a function definition may -#' span before being flagged. Defaults to 150, matching the documented -#' heuristic. -#' @return A [lintr::Linter()] object. -#' @export -function_length_linter <- function(length_limit = 150L) { - xpath <- "//FUNCTION/parent::expr | //OP-LAMBDA/parent::expr" - - lintr::Linter(linter_level = "expression", function(source_expression) { - if (!lintr::is_lint_level(source_expression, "expression")) { - return(list()) - } - - xml <- source_expression$xml_parsed_content - fun_defs <- xml2::xml_find_all(xml, xpath) - - n_lines <- as.integer(xml2::xml_attr(fun_defs, "line2")) - - as.integer(xml2::xml_attr(fun_defs, "line1")) + 1L - - lintr::xml_nodes_to_lints( - fun_defs[n_lines > length_limit], - source_expression = source_expression, - lint_message = sprintf( - paste0( - "Function spans more than %d lines; consider decomposing it (see ", - "https://morrison-lab.github.io/psc/chapters/", - "coding-practices.html#sec-function-length-limits)." - ), - length_limit - ), - type = "warning" - ) - }) -} - -#' Default UCD-SERG lintr linters -#' -#' The canonical linter set for UCD-SERG repositories, matching the style -#' guide in the lab manual. Applied repositories consume this from a thin -#' `.lintr.R`: -#' -#' ```r -#' linters <- lms::default_linters() -#' exclusions <- list(...) # repo-specific, stays local -#' ``` -#' -#' @return A list of linters, as produced by -#' [lintr::linters_with_defaults()]. -#' @export -default_linters <- function() { - lintr::linters_with_defaults( - return_linter = NULL, - trailing_whitespace_linter = NULL, - lintr::redundant_equals_linter(), - lintr::pipe_consistency_linter(pipe = "|>"), - lintr::object_name_linter( - regexes = c(snake_case_ACROs1 = snake_case_acros_regex()) - ), - lintr::undesirable_function_linter( - fun = undesirable_functions(), - symbol_is_undesirable = TRUE - ), - lintr::cyclocomp_linter(), - function_length_linter(), - function_location_linter(), - condition_class_linter() - ) -} diff --git a/lms/R/match_expectation_args.R b/lms/R/match_expectation_args.R new file mode 100644 index 00000000..4fbf0e29 --- /dev/null +++ b/lms/R/match_expectation_args.R @@ -0,0 +1,31 @@ +#' Which leading testthat expectation formals a call supplies +#' +#' Matches a call's arguments to the first three formals of +#' `expect_error()`, `expect_warning()`, and `expect_message()` +#' (`object`, `regexp`, `class`) the way R does: +#' named arguments first, by exact or unique partial match, +#' then unnamed arguments, in order, to the formals still unmatched. +#' All three formals come before `...`, so partial matching applies to them. +#' +#' @param call The call's `expr` node in lintr's XML parse tree. +#' @return A character vector of the formals among +#' `object`, `regexp`, and `class` that the call supplies. +#' @noRd +match_expectation_args <- function(call) { + formals <- c("object", "regexp", "class") + + arg_names <- xml2::xml_text(xml2::xml_find_all(call, "./SYMBOL_SUB")) + matches <- pmatch(arg_names, formals) + named <- formals[matches[!is.na(matches)]] + + # An unnamed argument is an `expr` child right after `(` or `,`; + # a named argument's value follows `EQ_SUB` instead. + n_unnamed <- length(xml2::xml_find_all( + call, + "./expr[preceding-sibling::*[1][self::OP-LEFT-PAREN or self::OP-COMMA]]" + )) + unmatched <- setdiff(formals, named) + unnamed <- unmatched[seq_len(min(n_unnamed, length(unmatched)))] + + c(named, unnamed) +} diff --git a/lms/R/snake_case_acros_regex.R b/lms/R/snake_case_acros_regex.R new file mode 100644 index 00000000..00608cf1 --- /dev/null +++ b/lms/R/snake_case_acros_regex.R @@ -0,0 +1,26 @@ +#' snake_case object-name regex (uppercase acronyms allowed) +#' +#' Builds the regex used by the object-name linter so that names like +#' `snake_case_ACROs1` (uppercase acronyms, optional trailing digits) are +#' accepted. See for background. +#' +#' Constructed with `rex::rex()`, which evaluates its arguments in an +#' environment that registers the rex shortcuts (`start`, `upper`, `digit`, +#' `%or%`, ...), so the package does **not** need to attach rex. +#' +#' @return A single regular-expression string. +#' @noRd +snake_case_acros_regex <- function() { + rex::rex( + start, + maybe("."), + list(some_of(upper), maybe("s"), zero_or_more(digit)) %or% + list(some_of(lower), zero_or_more(digit)), + zero_or_more( + "_", + list(some_of(upper), maybe("s"), zero_or_more(digit)) %or% + list(some_of(lower), zero_or_more(digit)) + ), + end + ) +} diff --git a/lms/R/undesirable_functions.R b/lms/R/undesirable_functions.R new file mode 100644 index 00000000..c3a6d063 --- /dev/null +++ b/lms/R/undesirable_functions.R @@ -0,0 +1,47 @@ +#' Undesirable functions for UCD-SERG repositories +#' +#' The default lintr undesirable-function map, extended with the UCD-SERG +#' conventions: base and rlang messaging functions are steered toward their +#' `cli::` equivalents, `library()` is discouraged inside package code, and a +#' few of lintr's defaults (`structure()`, `browser()`) are dropped. +#' +#' Exported so a downstream repository can further `lintr::modify_defaults()` +#' it if a single repo needs to diverge, without copy-pasting the whole map. +#' +#' @return A named character vector suitable for +#' [lintr::undesirable_function_linter()]'s `fun` argument. +#' @export +undesirable_functions <- function() { + lintr::modify_defaults( + defaults = lintr::default_undesirable_functions, + + # following https://github.com/r-lib/devtools/blob/2aa51ef/.lintr.R: + # 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()", + + library = paste( + "\nuse `::`, `usethis::use_import_from()`, or `withr::local_package()`", + "instead of modifying the global search path.", + "\nSee:\n", + " and\n", + "", # nolint: line_length_linter. + "\nfor more details." + ), + + structure = NULL, + browser = NULL + # see https://github.com/r-lib/lintr/pull/2227 and + # rebuttal https://github.com/r-lib/lintr/pull/2227#issuecomment-1800302675 + ) +} diff --git a/lms/README.md b/lms/README.md index 942673a4..d47f0769 100644 --- a/lms/README.md +++ b/lms/README.md @@ -1,8 +1,11 @@ # lms --- Lab Manual Style -`lms` ("lab manual style") is the shared UCD-SERG [`{lintr}`](https://lintr.r-lib.org/) -configuration, packaged as a small R package so every lab repository enforces the -**same** linter set without copy-pasting `.lintr.R` files (which drift apart over time). +`lms` ("lab manual style") is the shared UCD-SERG +[`{lintr}`](https://lintr.r-lib.org/) configuration. +It is packaged as a small R package +so every lab repository enforces the **same** linter set +without copy-pasting `.lintr.R` files, +which drift apart over time. It lives in the `lms/` subdirectory of [`Morrison-Lab/psc`](https://github.com/Morrison-Lab/psc), @@ -13,17 +16,21 @@ It was written for, and first kept in, the and moved here with its history in October 2026. It exports: -- `default_linters()` --- the canonical linter set (`lintr::linters_with_defaults()` - plus the lab's rules: `|>` pipe consistency, snake_case-with-acronyms object names, - `cli::`-oriented undesirable functions, function-location/length checks, - and condition-class assertions for `testthat` expectations that match `regexp`). -- `undesirable_functions()` --- the underlying undesirable-function map, if a repo needs - to extend it. +- `default_linters()` --- the canonical linter set: + `lintr::linters_with_defaults()` plus the lab's rules. + Those rules are `|>` pipe consistency, + snake_case-with-acronyms object names, + `cli::`-oriented undesirable functions, + function-location and function-length checks, + and condition-class assertions for `testthat` expectations that match `regexp`. +- `undesirable_functions()` --- the underlying undesirable-function map, + if a repo needs to extend it. ## Using lms in a repository -Each applied repo keeps a thin `.lintr.R` that calls `lms::default_linters()` and -supplies its **own** local exclusions: +Each applied repo keeps a thin `.lintr.R` +that calls `lms::default_linters()` +and supplies its **own** local exclusions: ```r # .lintr.R @@ -31,7 +38,9 @@ if (!requireNamespace("lms", quietly = TRUE)) { stop( # nolint: undesirable_function_linter. .lintr.R is config, not package code. "Package 'lms' is required to lint this project. Install it with:\n", " install.packages('remotes')\n", - " remotes::install_github('Morrison-Lab/psc/lms@lms-v0.1.1')", + " remotes::install_github(\n", + " 'Morrison-Lab/psc/lms@3d92ab2bb1115231ebee5ef071e2f4669b160945'\n", + " )", call. = FALSE ) } @@ -43,46 +52,66 @@ exclusions <- list( ) ``` -Install `lms` in CI with an **explicit** step in the lint workflow, before linting: +Install `lms` in CI with an **explicit** step in the lint workflow, +before linting: ```yaml - name: Install lms (shared linter config) run: | install.packages("remotes") - remotes::install_github("Morrison-Lab/psc/lms@lms-v0.1.1") + remotes::install_github( + "Morrison-Lab/psc/lms@3d92ab2bb1115231ebee5ef071e2f4669b160945" + ) shell: Rscript {0} ``` -For local linting, install `lms` once the same way: -`remotes::install_github("Morrison-Lab/psc/lms@lms-v0.1.1")`. +For local linting, install `lms` once the same way. -## Do NOT declare lms in DESCRIPTION - -`lms` is **lint-only tooling, not a runtime dependency** --- a linter configuration, not -something your package calls. Keep it out of `Imports`/`Suggests`, and do not -add a `Remotes:` entry for it. Two reasons: +The commit in these examples installs lms 0.1.1. +Pin a full commit SHA of psc `main`, as here, +until `lms` has release tags (see [Versioning](#versioning)). -1. **`{pak}` cannot resolve the subdir Remote.** The reference - `Morrison-Lab/psc/lms@lms-v0.1.1` (a package in a repo *subdirectory*) resolves with - the `{remotes}` package but **not** with `{pak}` / - `r-lib/actions/setup-r-dependencies`, which fail with `Can't find package called - lms`. If `lms` is in `Suggests`, that failure breaks *every* `{pak}`-based workflow in - the consuming repo (e.g. a bibliography/DOI check), even ones unrelated to linting. -2. **CRAN hygiene.** `.lintr.R` is a dev-time config file, not test code, so - `R CMD check` never executes it --- there's no CRAN footprint, and a linter config - has no place in a package's declared dependency graph. +## Do NOT declare lms in DESCRIPTION -The version pin lives in the CI `install_github(...@)` step, not in DESCRIPTION. +`lms` is **lint-only tooling, not a runtime dependency**: +it is a linter configuration, not something your package calls. +Keep it out of `Imports` and `Suggests`, +and do not add a `Remotes:` entry for it. +There are two reasons: + +1. **`{pak}` cannot resolve the subdir Remote.** + A reference to a package in a repo *subdirectory*, + such as `Morrison-Lab/psc/lms@`, + resolves with the `{remotes}` package + but **not** with `{pak}` or `r-lib/actions/setup-r-dependencies`, + which fail with `Can't find package called lms`. + If `lms` is in `Suggests`, + that failure breaks *every* `{pak}`-based workflow in the consuming repo + (for example, a bibliography/DOI check), + even ones unrelated to linting. +2. **CRAN hygiene.** + `.lintr.R` is a dev-time config file, not test code, + so `R CMD check` never executes it. + There is no CRAN footprint, + and a linter config has no place in a package's declared dependency graph. + +The version pin lives in the CI `install_github(...@)` step, +not in DESCRIPTION. ## Versioning -`lms` is released as a git **tag** on `psc` `main`, -named `lms-v` plus the `Version:` in `lms/DESCRIPTION` (currently `lms-v0.1.1`). +`lms` releases are meant to be git **tags** on psc `main`, +named `lms-v` plus the `Version:` in `lms/DESCRIPTION` +(for example, `lms-v0.1.2`). The `lms-` prefix keeps these tags apart from anything else the repository tags. +No `lms-v` tag exists yet, +so pin a commit SHA in the meantime. + To roll out a rule change: merge it to `main`, bump `Version:`, cut a new tag, -then bump the `@` ref in each consuming repo's lint workflow when ready. -Pinning to a tag (not `main`) means routine edits to the guide never churn downstream linting. +then bump the `@` in each consuming repo's lint workflow when ready. +Pinning to a tag or commit (not `main`) +means routine edits to the guide never churn downstream linting. Releases before the move are tags on the lab manual (`remotes::install_github("UCD-SERG/lab-manual/lms@v0.1.0")`), @@ -90,8 +119,9 @@ and they still install from there. ## Developing lms -Because `default_linters()` includes `object_usage_linter()`, lint **this** package -with the source loaded so symbols (and `.onLoad()` side effects) resolve correctly. +Because `default_linters()` includes `object_usage_linter()`, +lint **this** package with the source loaded +so symbols (and `.onLoad()` side effects) resolve correctly. From the `lms/` directory, run: ```r @@ -107,6 +137,7 @@ which excludes `lms/` and so reports no lints at all. psc's `lint-lms` CI job runs the same two calls from the repository root, passing `"lms"` as the path to each. -`.onLoad()` calls `rex::register_shortcuts()` so the `{rex}` DSL tokens used in the -snake_case regex are not flagged as undefined globals. After editing `R/` roxygen -comments, run `devtools::document()`. +`.onLoad()` calls `rex::register_shortcuts()` +so the `{rex}` DSL tokens used in the snake_case regex +are not flagged as undefined globals. +After editing `R/` roxygen comments, run `devtools::document()`. diff --git a/lms/man/default_linters.Rd b/lms/man/default_linters.Rd index 9c89f2b8..9dbcd5ab 100644 --- a/lms/man/default_linters.Rd +++ b/lms/man/default_linters.Rd @@ -1,5 +1,5 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/linters.R +% Please edit documentation in R/default_linters.R \name{default_linters} \alias{default_linters} \title{Default UCD-SERG lintr linters} diff --git a/lms/man/function_length_linter.Rd b/lms/man/function_length_linter.Rd index 5a50743e..82d5bdc9 100644 --- a/lms/man/function_length_linter.Rd +++ b/lms/man/function_length_linter.Rd @@ -1,5 +1,5 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/linters.R +% Please edit documentation in R/function_length_linter.R \name{function_length_linter} \alias{function_length_linter} \title{Function-length linter} diff --git a/lms/man/undesirable_functions.Rd b/lms/man/undesirable_functions.Rd index af104849..120a6fdc 100644 --- a/lms/man/undesirable_functions.Rd +++ b/lms/man/undesirable_functions.Rd @@ -1,5 +1,5 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/linters.R +% Please edit documentation in R/undesirable_functions.R \name{undesirable_functions} \alias{undesirable_functions} \title{Undesirable functions for UCD-SERG repositories} diff --git a/lms/tests/testthat/test-condition_class_linter.R b/lms/tests/testthat/test-condition_class_linter.R index c46674ac..c0ebdf0c 100644 --- a/lms/tests/testthat/test-condition_class_linter.R +++ b/lms/tests/testthat/test-condition_class_linter.R @@ -55,3 +55,44 @@ test_that("expect_error without regexp or with info is not flagged", { lms:::condition_class_linter() ) }) + +test_that("a positional class after a positional regexp is allowed", { + lintr::expect_no_lint( + 'expect_error(f(), "bad input", "my_pkg_error")', + lms:::condition_class_linter() + ) + lintr::expect_no_lint( + 'expect_warning(f(), "deprecated", "my_pkg_warning")', + lms:::condition_class_linter() + ) +}) + +test_that("a positional argument fills class when regexp is named", { + lintr::expect_no_lint( + 'expect_error(f(), regexp = "bad input", "my_pkg_error")', + lms:::condition_class_linter() + ) +}) + +test_that("a partially named class is allowed", { + lintr::expect_no_lint( + 'expect_error(f(), "bad input", cla = "my_pkg_error")', + lms:::condition_class_linter() + ) +}) + +test_that("a positional regexp after a named object is flagged", { + lintr::expect_lint( + 'expect_error(object = f(), "bad input")', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +}) + +test_that("info is not mistaken for class", { + lintr::expect_lint( + 'expect_error(f(), "bad input", info = "details")', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +}) From 118adb380901044563cb18471f2c1db1c1a5e584 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 22:15:42 +0000 Subject: [PATCH 2/3] Match expectation arguments the way R does in more cases The self-review of 58c6168 found calls the matcher misread: a piped object, a comment between arguments, a backticked or quoted argument name, an empty argument, and a forwarded `...`. call_arg_slots() now splits the call into argument slots, skipping comments and stripping name delimiters. match_expectation_args() counts the piped value as the first unnamed argument, lets an empty argument take its slot without supplying it, and treats `...` as possibly supplying anything. Nine of the new expectations fail on 58c6168's matcher. The README now says the pinned 0.1.1 commit predates the fix. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Jt6yVGbqCGUgWio5QUBDex --- lms/R/call_arg_slots.R | 41 +++++++++++++ lms/R/match_expectation_args.R | 34 ++++++++--- lms/README.md | 3 +- .../testthat/test-condition_class_linter.R | 61 +++++++++++++++++++ 4 files changed, 128 insertions(+), 11 deletions(-) create mode 100644 lms/R/call_arg_slots.R diff --git a/lms/R/call_arg_slots.R b/lms/R/call_arg_slots.R new file mode 100644 index 00000000..68916f88 --- /dev/null +++ b/lms/R/call_arg_slots.R @@ -0,0 +1,41 @@ +#' Split a call's arguments into slots +#' +#' Walks the children of a call's `expr` node in lintr's XML parse tree, +#' between its parentheses, and splits them at each `OP-COMMA`. +#' Comments are skipped. +#' A slot is named when it holds `EQ_SUB`; +#' the name drops surrounding backticks or quotes, +#' so `` `class` = x `` and `"class" = x` both name `class`. +#' +#' @param call The call's `expr` node in lintr's XML parse tree. +#' @return A list with one element per argument slot, each a list with +#' `name` (the argument name, or `NA` when unnamed) and +#' `text` (the argument value's source text, `""` for an empty argument). +#' @noRd +call_arg_slots <- function(call) { + children <- xml2::xml_children(call) + kinds <- xml2::xml_name(children) + inside <- seq_along(kinds) > match("OP-LEFT-PAREN", kinds) & + seq_along(kinds) < length(kinds) & + kinds != "COMMENT" + args <- children[inside] + arg_kinds <- kinds[inside] + if (!length(args)) { + return(list()) + } + + slot_id <- cumsum(arg_kinds == "OP-COMMA") + lapply(split(seq_along(args), slot_id), function(idx) { + idx <- idx[arg_kinds[idx] != "OP-COMMA"] + eq <- which(arg_kinds[idx] == "EQ_SUB") + if (length(eq)) { + name <- gsub("^[`'\"]|[`'\"]$", "", xml2::xml_text(args[[idx[1]]])) + value <- idx[-seq_len(eq[1])] + } else { + name <- NA_character_ + value <- idx + } + text <- paste(xml2::xml_text(args[value]), collapse = "") + list(name = name, text = text) + }) +} diff --git a/lms/R/match_expectation_args.R b/lms/R/match_expectation_args.R index 4fbf0e29..aa3b6c65 100644 --- a/lms/R/match_expectation_args.R +++ b/lms/R/match_expectation_args.R @@ -7,25 +7,39 @@ #' then unnamed arguments, in order, to the formals still unmatched. #' All three formals come before `...`, so partial matching applies to them. #' +#' A call on the right of `|>` gets the piped value as its first unnamed +#' argument. +#' An empty argument, as in `f(x, , y)`, takes its slot but supplies nothing. +#' A forwarded `...` could supply any formal, +#' so a call containing one is reported as supplying all three. +#' #' @param call The call's `expr` node in lintr's XML parse tree. #' @return A character vector of the formals among #' `object`, `regexp`, and `class` that the call supplies. #' @noRd match_expectation_args <- function(call) { formals <- c("object", "regexp", "class") + slots <- call_arg_slots(call) + + texts <- vapply(slots, function(slot) slot$text, character(1)) + if ("..." %in% texts) { + return(formals) + } - arg_names <- xml2::xml_text(xml2::xml_find_all(call, "./SYMBOL_SUB")) - matches <- pmatch(arg_names, formals) + arg_names <- vapply(slots, function(slot) slot$name, character(1)) + unnamed_texts <- texts[is.na(arg_names)] + matches <- pmatch(arg_names[!is.na(arg_names)], formals) named <- formals[matches[!is.na(matches)]] - # An unnamed argument is an `expr` child right after `(` or `,`; - # a named argument's value follows `EQ_SUB` instead. - n_unnamed <- length(xml2::xml_find_all( - call, - "./expr[preceding-sibling::*[1][self::OP-LEFT-PAREN or self::OP-COMMA]]" - )) + previous <- xml2::xml_find_first(call, "preceding-sibling::*[1]") + if (identical(xml2::xml_name(previous), "PIPE")) { + unnamed_texts <- c("", unnamed_texts) + } + unmatched <- setdiff(formals, named) - unnamed <- unmatched[seq_len(min(n_unnamed, length(unmatched)))] + n_filled <- min(length(unnamed_texts), length(unmatched)) + filled <- unmatched[seq_len(n_filled)] + supplied <- nzchar(unnamed_texts[seq_len(n_filled)]) - c(named, unnamed) + c(named, filled[supplied]) } diff --git a/lms/README.md b/lms/README.md index d47f0769..9a3c15e3 100644 --- a/lms/README.md +++ b/lms/README.md @@ -67,7 +67,8 @@ before linting: For local linting, install `lms` once the same way. -The commit in these examples installs lms 0.1.1. +The commit in these examples installs lms 0.1.1, +which predates 0.1.2's handling of a positional `class` argument. Pin a full commit SHA of psc `main`, as here, until `lms` has release tags (see [Versioning](#versioning)). diff --git a/lms/tests/testthat/test-condition_class_linter.R b/lms/tests/testthat/test-condition_class_linter.R index c0ebdf0c..7c0232c9 100644 --- a/lms/tests/testthat/test-condition_class_linter.R +++ b/lms/tests/testthat/test-condition_class_linter.R @@ -96,3 +96,64 @@ test_that("info is not mistaken for class", { lms:::condition_class_linter() ) }) + +test_that("a piped object counts as the first argument", { + lintr::expect_no_lint( + 'f() |> expect_error("bad input", "my_pkg_error")', + lms:::condition_class_linter() + ) + lintr::expect_lint( + 'f() |> expect_error("bad input")', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +}) + +test_that("comments between arguments do not hide them", { + lintr::expect_no_lint( + 'expect_error(f(), "bad input", # why\n "my_pkg_error")', + lms:::condition_class_linter() + ) + lintr::expect_lint( + 'expect_error(f(), # why\n "bad input")', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +}) + +test_that("backticked and quoted argument names are matched", { + lintr::expect_no_lint( + 'expect_error(f(), "bad input", `class` = "my_pkg_error")', + lms:::condition_class_linter() + ) + lintr::expect_no_lint( + 'expect_error(f(), "bad input", "class" = "my_pkg_error")', + lms:::condition_class_linter() + ) + lintr::expect_lint( + 'expect_error(f(), `regexp` = "bad input")', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +}) + +test_that("an empty argument takes its slot but supplies nothing", { + lintr::expect_no_lint( + 'expect_error(f(), , "my_pkg_error")', + lms:::condition_class_linter() + ) +}) + +test_that("a forwarded ... is not flagged", { + lintr::expect_no_lint( + "check <- function(...) expect_error(f(), ...)", + lms:::condition_class_linter() + ) +}) + +test_that("expect_message with a positional class is allowed", { + lintr::expect_no_lint( + 'expect_message(f(), "informative", "my_pkg_message")', + lms:::condition_class_linter() + ) +}) From cc33a35c8e5706ba9b9aba8ac97db79b7386566c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 22:19:03 +0000 Subject: [PATCH 3/3] Read magrittr pipes, placeholders and empty arguments correctly The second self-review found cases the matcher still misread. piped_into() now recognizes magrittr pipes (%>%, %T>%, %<>%) as well as |>, skipping comments before the call. A placeholder (`_` for |>, `.` for magrittr) keeps the piped value out of the positional slots. A named argument with an empty value takes its formal but supplies nothing, and a leading empty argument keeps its slot. All six new expectations fail on 118adb3. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01Jt6yVGbqCGUgWio5QUBDex --- lms/R/call_arg_slots.R | 3 ++ lms/R/match_expectation_args.R | 34 +++++++++++---- lms/R/piped_into.R | 25 +++++++++++ .../testthat/test-condition_class_linter.R | 43 +++++++++++++++++++ 4 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 lms/R/piped_into.R diff --git a/lms/R/call_arg_slots.R b/lms/R/call_arg_slots.R index 68916f88..9e7cd5b7 100644 --- a/lms/R/call_arg_slots.R +++ b/lms/R/call_arg_slots.R @@ -24,7 +24,10 @@ call_arg_slots <- function(call) { return(list()) } + # A factor keeps an empty slot (such as the first in `f(, x)`), which + # would otherwise have no elements and be dropped by split(). slot_id <- cumsum(arg_kinds == "OP-COMMA") + slot_id <- factor(slot_id, levels = 0:max(slot_id)) lapply(split(seq_along(args), slot_id), function(idx) { idx <- idx[arg_kinds[idx] != "OP-COMMA"] eq <- which(arg_kinds[idx] == "EQ_SUB") diff --git a/lms/R/match_expectation_args.R b/lms/R/match_expectation_args.R index aa3b6c65..d1fc983f 100644 --- a/lms/R/match_expectation_args.R +++ b/lms/R/match_expectation_args.R @@ -7,9 +7,12 @@ #' then unnamed arguments, in order, to the formals still unmatched. #' All three formals come before `...`, so partial matching applies to them. #' -#' A call on the right of `|>` gets the piped value as its first unnamed -#' argument. -#' An empty argument, as in `f(x, , y)`, takes its slot but supplies nothing. +#' A call on the right of `|>` or a magrittr pipe (`%>%`, `%T>%`, `%<>%`) +#' gets the piped value as its first unnamed argument, +#' unless the call places it with a placeholder: +#' `_` as a named argument for `|>`, or `.` as any argument for magrittr. +#' An empty argument, as in `f(x, , y)` or `f(x, regexp = )`, +#' takes its slot but supplies nothing. #' A forwarded `...` could supply any formal, #' so a call containing one is reported as supplying all three. #' @@ -27,16 +30,29 @@ match_expectation_args <- function(call) { } arg_names <- vapply(slots, function(slot) slot$name, character(1)) - unnamed_texts <- texts[is.na(arg_names)] - matches <- pmatch(arg_names[!is.na(arg_names)], formals) - named <- formals[matches[!is.na(matches)]] + is_named <- !is.na(arg_names) + unnamed_texts <- texts[!is_named] + matches <- pmatch(arg_names[is_named], formals) + hit <- !is.na(matches) + matched <- formals[matches[hit]] + # A named argument with an empty value (`regexp = `) takes its formal + # but supplies nothing. + named <- matched[nzchar(texts[is_named][hit])] - previous <- xml2::xml_find_first(call, "preceding-sibling::*[1]") - if (identical(xml2::xml_name(previous), "PIPE")) { + # A pipe passes its left side as the first unnamed argument, + # unless the call places it with a placeholder: + # `_` in a named argument for `|>`, or `.` in any argument for magrittr. + pipe <- piped_into(call) + placed <- if (identical(pipe, "|>")) { + "_" %in% texts[is_named] + } else { + "." %in% texts + } + if (!is.na(pipe) && !placed) { unnamed_texts <- c("", unnamed_texts) } - unmatched <- setdiff(formals, named) + unmatched <- setdiff(formals, matched) n_filled <- min(length(unnamed_texts), length(unmatched)) filled <- unmatched[seq_len(n_filled)] supplied <- nzchar(unnamed_texts[seq_len(n_filled)]) diff --git a/lms/R/piped_into.R b/lms/R/piped_into.R new file mode 100644 index 00000000..255a953c --- /dev/null +++ b/lms/R/piped_into.R @@ -0,0 +1,25 @@ +#' Which pipe, if any, feeds a call +#' +#' Looks at the token just before a call's `expr` node, skipping comments. +#' The base pipe is a `PIPE` token; +#' a magrittr pipe is a `SPECIAL` token such as `%>%`. +#' +#' @param call The call's `expr` node in lintr's XML parse tree. +#' @return `"|>"`, the magrittr pipe's text (`"%>%"`, `"%T>%"` or `"%<>%"`), +#' or `NA` when the call is not on the right of a pipe. +#' @noRd +piped_into <- function(call) { + previous <- xml2::xml_find_first( + call, + "preceding-sibling::*[not(self::COMMENT)][1]" + ) + kind <- xml2::xml_name(previous) + text <- xml2::xml_text(previous) + if (identical(kind, "PIPE")) { + return("|>") + } + if (identical(kind, "SPECIAL") && text %in% c("%>%", "%T>%", "%<>%")) { + return(text) + } + NA_character_ +} diff --git a/lms/tests/testthat/test-condition_class_linter.R b/lms/tests/testthat/test-condition_class_linter.R index 7c0232c9..0ac554d7 100644 --- a/lms/tests/testthat/test-condition_class_linter.R +++ b/lms/tests/testthat/test-condition_class_linter.R @@ -157,3 +157,46 @@ test_that("expect_message with a positional class is allowed", { lms:::condition_class_linter() ) }) + +test_that("a magrittr pipe supplies the object", { + lintr::expect_no_lint( + 'f() %>% expect_error("bad input", "my_pkg_error")', + lms:::condition_class_linter() + ) + lintr::expect_lint( + 'f() %>% expect_error("bad input")', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +}) + +test_that("a comment between the pipe and the call is skipped", { + lintr::expect_lint( + 'f() |> # comment\n expect_error("bad input")', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +}) + +test_that("a pipe placeholder does not shift the arguments", { + lintr::expect_lint( + 'f |> expect_error("bad input", object = _)', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +}) + +test_that("a named argument with an empty value supplies nothing", { + lintr::expect_no_lint( + "expect_error(f(), regexp = )", + lms:::condition_class_linter() + ) +}) + +test_that("a leading empty argument takes the object slot", { + lintr::expect_lint( + 'expect_error(, "bad input")', + "include a `class =` assertion", + lms:::condition_class_linter() + ) +})