Split lms linters into one file each and match expectation arguments as R does

[dem-extra1]

Before: lms/R/linters.R held four unrelated objects in one file, and condition_class_linter() only recognized a class argument passed by name. It flagged correct calls such as expect_error(f(), "msg", "my_class") and f() |> expect_error("msg", class = "c"), and it missed calls whose regexp was partially named or piped.

After: each function has its own file, and the linter matches an expectation's object, regexp and class arguments the way R does. That covers named, partially named and positional arguments, |> and magrittr pipes and their placeholders, comments between arguments, quoted or backticked names, empty arguments, and a forwarded ....

How: call_arg_slots() splits a call's parse tree into argument slots, piped_into() names the pipe feeding a call, and match_expectation_args() applies R's matching order (exact or partial names, then positions) to the three formals. lms is bumped to 0.1.2, and the README now says the pinned 0.1.1 commit predates this fix.

Each round of tests was checked against the previous commit: the new expectations fail there and pass here. Three rounds of adversarial self-review ended CLEAN on cc33a35.

Closes #32, closes #34, closes #35.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Jt6yVGbqCGUgWio5QUBDex

---

Generated by Claude Code

[github-actions]

Claude finished review — View run

---

Clean. This completes my review. The implementation is well-tested, the logic traces correctly through every test case I checked by hand, documentation/links are accurate, issue references are real, and the referenced commit SHA in the README genuinely predates the fix. I found no blocking issues.

---

Review: #40 — Clear the lms issues deferred from the move

Scope. Entirely within lms/ (the vendored lintr-config R package): splits R/linters.R into one file per function (closes #35), rewrites condition_class_linter()'s argument matching to follow R's actual argument-matching rules via two new helpers, call_arg_slots() and match_expectation_args()/piped_into() (closes #34), bumps lms to 0.1.2, and re-wraps lms/README.md with corrected install instructions (closes #32). No .qmd, site config, or _site/_freeze changes — consistent with the stated scope.

Correctness. I don't have R/lintr/xml2 available in this sandbox to execute the test suite, so I hand-traced the new matching logic (call_arg_slots.R, match_expectation_args.R, piped_into.R) against ~15 of the new test cases in test-condition_class_linter.R (comments between arguments, backticked/quoted names, empty-argument slots, |> and magrittr pipes with and without placeholders, forwarded ..., positional-after-named filling). In every case I traced, the implementation's output matches the test's expectation, and the logic correctly mirrors R's own argument-matching order (named exact/partial match first, reserving the formal even when the value is empty, then unnamed arguments filling remaining formals in declared order). No pre-existing tests were removed (verified via git diff origin/main), only 11 new test_that() blocks added.

Build hygiene / consistency checks performed:

• All regenerated man/*.Rd headers correctly point at the new source file paths (R/default_linters.R, R/function_length_linter.R, R/undesirable_functions.R); no stale R/linters.R references remain anywhere in man/.
• NAMESPACE is untouched and still correct — no exports were added or removed, matching the pure refactor.
• Each new file has exactly one top-level function, matching psc's "One function per file" convention, which is real and documented (chapters/coding-practices.qmd:87), supporting the PR description's claim.
• Both doc links the linter messages point to (#sec-function-length-limits, #sec-r-workflow) resolve to real anchors in chapters/coding-practices.qmd.
• The README's pinned commit SHA (3d92ab2bb1115231ebee5ef071e2f4669b160945) is real (fetched and confirmed it exists on origin) and its lms/DESCRIPTION at that commit does read Version: 0.1.1, so the claim "installs lms 0.1.1" is accurate.
• Issues Re-break lms/README.md with semantic line breaks #32, lms: condition_class_linter misses a positional class argument #34, lms: split R/linters.R into one function per file #35 cited by "Closes" are real, open, and match the PR's described fixes.
• The one line over 80 chars (lms/R/undesirable_functions.R:38) is unchanged, moved-verbatim content already carrying its pre-existing # nolint: line_length_linter. marker — not a new violation.
• roxygen prose consistently backticks pkg::fn() references per the lab manual's coding-style guidance.

No AI hallucinations found (URLs, SHAs, and issue numbers all check out), no CLAUDE.md violations, no accidental _site/_freeze commits, no new dependencies.

Verdict

Ready for merge

Structured Review Data (JSON)

{
  "schema_version": "1.1",
  "reviewer": "claude",
  "commit_sha": "02eafe928317d616ba5a0eb1b172e4208234f018",
  "verdict": "CLEAN",
  "findings": [],
  "detailed_assessment": "Hand-traced match_expectation_args.R, call_arg_slots.R, and piped_into.R against roughly fifteen new testthat cases covering pipes, placeholders, comments, and empty arguments without finding a divergence from R's own argument-matching order.",
  "holistic_assessment": "Verified man page source headers, NAMESPACE exports, referenced issue numbers, and the pinned install commit SHA all stayed consistent after the R/linters.R file split and version bump to 0.1.2."
}

Reviewed commit: 903973b

[github-actions]

💰 Cost: $1.0220 (review) — run

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.

_{2026-10-16 03:34 UTC}

[d-morrison]

agy review of 903973b (model: Gemini 4 Argon (High), gemini-4-argon-high):

Adversarial Review of Morrison-Lab/psc#40 (903973b655929ee975f0bfb2b9280498622d7878)

1. Argument matching (call_arg_slots(), piped_into(), match_expectation_args())

• Formals & R matching order (lms/R/match_expectation_args.R:23-61):
  The three target expectations (testthat::expect_error(), testthat::expect_warning(), and testthat::expect_message()) all share the same three leading formals before ...: (object, regexp = NULL, class = NULL, ...), with keyword-only formals (inherit, all, info, label) after ....

  • Because R only partially matches formals that precede ..., and none of the post-... formals (inherit, all, info, label) is a prefix of object, regexp, or class, pmatch(arg_names[is_named], c("object", "regexp", "class")) accurately reproduces R's exact-then-partial matching for the three target formals while ignoring info =, inherit =, label =, all =, and arbitrary ... named arguments.
  • Empty named arguments (regexp = ) still reserve their formal in matched (so subsequent positional arguments skip regexp and fill class), while named <- matched[nzchar(texts[is_named][hit])] records that regexp was not actually supplied.
  • Empty positional arguments (expect_error(f(), , "my_class") or expect_error(, "bad")) are preserved via the factor(slot_id, levels = 0:max(slot_id)) split in call_arg_slots(), occupy their positional formal in filled, and are excluded by supplied <- nzchar(unnamed_texts[seq_len(n_filled)]).
  • Trailing commas (expect_error(f(), )) also work cleanly: the trailing OP-COMMA increments cumsum(arg_kinds == "OP-COMMA"), creating a trailing level whose OP-COMMA token is stripped, leaving an empty slot.
  • Comments inside the argument list are filtered out before splitting (kinds != "COMMENT" in lms/R/call_arg_slots.R:20), and comments between a pipe token and the RHS call are skipped via preceding-sibling::*[not(self::COMMENT)][1] in lms/R/piped_into.R:14.

• nit (lms/R/match_expectation_args.R:46-50) — Pipe placeholder inside an extraction expression (_$x):
  In R $\ge$ 4.3, the native pipe placeholder _ can be the head of an extraction chain in a named argument (e.g. spec |> expect_error(f(), regexp = _$pattern)). Because placed checks "_" %in% texts[is_named] for exact equality against "_", a slot whose text is "_$pattern" evaluates to placed = FALSE, causing "<piped value>" to be prepended to unnamed_texts alongside f() so f() shifts into class. Checking for a PLACEHOLDER token inside named argument nodes (or grepl("^_", texts[is_named])) would cover extraction chains too, though this pattern is rare in expect_error() calls.

2. condition_class_linter() behavior and tests (lms/R/condition_class_linter.R, lms/tests/testthat/test-condition_class_linter.R)

• condition_class_linter() flags a call iff "regexp" %in% supplied && !("class" %in% supplied).
• Each of the 16 new test_that() blocks in lms/tests/testthat/test-condition_class_linter.R:59-202 directly exercises a distinct branch of the new logic (positional class after positional or named regexp, partial naming cla =, positional regexp after object =, info = non-confusion, |> and %>% pipes with and without comments or _, backticked/quoted argument names, empty positional and named slots, and forwarded ...) and would fail against the pre-PR SYMBOL_SUB / ./expr[3] implementation.

3. File split, roxygen/man, and DESCRIPTION

• All four functions moved out of lms/R/linters.R (undesirable_functions.R, snake_case_acros_regex.R, function_length_linter.R, default_linters.R) are byte-for-byte identical to their definitions in the deleted lms/R/linters.R.
• The .Rd headers in lms/man/default_linters.Rd, lms/man/function_length_linter.Rd, and lms/man/undesirable_functions.Rd match devtools::document() output; condition_class_linter, call_arg_slots, match_expectation_args, piped_into, and snake_case_acros_regex all carry @noRd.
• lms/DESCRIPTION bumps Version from 0.1.1 to 0.1.2. No Collate field is needed since all R/*.R files only define top-level functions.

4. Prose accuracy (lms/README.md, roxygen, PR description)

• lms/README.md accurately replaces the nonexistent @lms-v0.1.1 tag reference with the 0.1.1 commit SHA (3d92ab2bb1115231ebee5ef071e2f4669b160945) and documents why a commit SHA is pinned until release tags are created.

5. R CMD check, self-linting, and CI

• All added R files adhere to the 80-character line limit, snake_case naming, function_location_linter() (one function per file matching the filename), and object_usage_linter() (xml2:: and lintr:: calls are namespace-qualified).

VERDICT: CLEAN

Posted by Claude Code (AI agent) --- not written by a human.

[dem-extra1]

The agy review's one nit (a |> placeholder inside an extraction, such as regexp = _$pattern) is deferred to #43. It is a rare form, and fixing it here would move the head away from the reviewed commit 903973b.

Posted by Claude Code (AI agent) --- not written by a human.

---

Generated by Claude Code