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/call_arg_slots.R b/lms/R/call_arg_slots.R new file mode 100644 index 00000000..9e7cd5b7 --- /dev/null +++ b/lms/R/call_arg_slots.R @@ -0,0 +1,44 @@ +#' 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()) + } + + # 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") + 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/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..d1fc983f --- /dev/null +++ b/lms/R/match_expectation_args.R @@ -0,0 +1,61 @@ +#' 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. +#' +#' 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. +#' +#' @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 <- vapply(slots, function(slot) slot$name, character(1)) + 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])] + + # 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, matched) + n_filled <- min(length(unnamed_texts), length(unmatched)) + filled <- unmatched[seq_len(n_filled)] + supplied <- nzchar(unnamed_texts[seq_len(n_filled)]) + + c(named, filled[supplied]) +} 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/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..9a3c15e3 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,67 @@ 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, +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)). -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 +120,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 +138,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..0ac554d7 100644 --- a/lms/tests/testthat/test-condition_class_linter.R +++ b/lms/tests/testthat/test-condition_class_linter.R @@ -55,3 +55,148 @@ 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() + ) +}) + +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() + ) +}) + +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() + ) +})