[d-morrison]

Requested by Ezra · project thread

Ezra's rule (2026-11-03): never nest aes() inside ggplot(), and pipe the primary data into a bare ggplot() rather than passing it as an argument. Form: df |> ggplot() + aes(x, y) + geom_line() (message).

nested_call_linter() allows aes() as an argument, so ggplot(df, aes(x, y)) passed lint. This adds ggplot_args_linter(), a warning on any argument of ggplot(): an aes() call gets the "don't nest aes()" message, and any other argument (ggplot(df), ggplot(data = df)) gets the "pipe the data" message. Positional, named and ggplot2:: forms are covered. aes() inside a layer such as geom_point(aes(colour = g)) is not flagged. It is included in default_linters(nested_calls = TRUE), so lint-changed-lines enforces it on changed lines once a repo's lms-ref moves to the merged commit. No existing code is swept.

Tests: 17 expectations in test-ggplot_args_linter.R (flagged forms, the data argument, both lints on one call, unflagged forms, inclusion in the default set), and the full lms suite passes. Not run: R CMD check.

The lds chunk that prompted this is fixed in lds#1752.

Annotations read on head f5471d9: all 16 check runs carry only notice annotations, none from this diff. They are the runner-images notice that the ubuntu-latest label migrates to Ubuntu 26, an empty SUBMODULES_TOKEN (submodules clone anonymously), and the preview's diff highlighting cap on a page with more than 500 elements. This change touches only lms/.

Cross-harness review waived: the agy and OpenCode Go quotas are exhausted and Codex is not used (standing waiver, adversarial-self-review.md). Merged on Ezra's mwc.

https://claude.ai/code/session_019xfc9nm2UYhuLY1k6irzLP

---

Generated by Claude Code

[github-actions]

Claude finished review — View run

---

Review: Add ggplot_args_linter() (PR #89)

Summary: Adds a new lintr-based linter, ggplot_args_linter(), to the lms package that flags any argument passed to ggplot()/ggplot2::ggplot(), with a distinct message for an aes() argument vs. any other (data) argument. It's wired into default_linters(nested_calls = TRUE), documented (roxygen + generated .Rd + README), and covered by 17 testthat expectations.

Correctness check (XPath logic)

I don't have R/lintr available in this sandbox to execute the tests directly, but I traced the XPath logic by hand against R's parse-data grammar and cross-checked it against the existing nested_call_linter.R in the same package (same expr[1]/SYMBOL_FUNCTION_CALL[text() = '...'] idiom for "called as name(...)", same position() > 1 idiom for "the actual argument exprs, skipping punctuation/SYMBOL_SUB/EQ_SUB tokens"):

• ggplot_call correctly matches both ggplot(...) and ggplot2::ggplot(...) forms, since a namespaced call's function-part expr still has a direct SYMBOL_FUNCTION_CALL child with the trailing name.
• is_aes / aes_xpath / data_xpath correctly partition arguments into "is an aes() call" vs. "everything else," matching all the test cases (bare symbol, named data =, named mapping = aes(...), nested non-aes call like read_csv(path)).
• The piped forms (df |> ggplot2::ggplot() + aes(x, y)) are correctly not flagged for the data argument, because R's native pipe is preserved as a distinct node in getParseData/lintr's XML rather than desugared into the call's argument list — this is independently confirmed by the pre-existing test-nested_call_linter.R expectation expect_no_lint("x |> g() |> f()", ...), which only makes sense under that same assumption. |> also binds tighter than +, so df |> ggplot2::ggplot() + aes(x, y) parses as (df |> ggplot2::ggplot()) + aes(x, y) as intended.
• aes() used inside a later layer (geom_point(aes(colour = g))) is correctly unaffected, since the xpath only matches under a ggplot-named call.

I verified the PR body's claim of "17 expectations" by counting expect_lint/expect_no_lint/expect_match/expect_length/expect_true/expect_false calls in the new test file: 4 + 4 + 3 + 2 + 2 + 2 = 17. Matches exactly, no inflation.

Lab-manual / CLAUDE.md compliance

• Object naming is snake_case throughout; line lengths in the new R file max out at 79 chars (under the implicit 80-char default).
• Backticks inside lint messages (`+ aes(x, y)`, `df |> ggplot() + aes(x, y)`) match the existing precedent in condition_class_linter.R (e.g. `expect_error()`), so this isn't a style deviation.
• NAMESPACE insertion is alphabetically correct (function_location_linter → ggplot_args_linter → nested_call_linter), consistent with roxygen2's auto-sort.
• Test file naming (test-ggplot_args_linter.R) matches the existing test-*_linter.R convention.
• No new dependencies introduced (reuses already-imported lintr/xml2).
• Scope is tight: only lms/ files touched (NAMESPACE, R source, generated .Rd, README, tests) — no unrelated refactors.
• lms/DESCRIPTION Version: is left at 0.2.1, un-bumped. Per lms/README.md's documented rollout flow ("merge it to main, bump Version:, cut a new tag, then bump the @<ref> ... when ready"), the version bump and tag are described as a separate, later step rather than something this PR must include, so I'm not flagging this as a gap.
• lms/.lintr.R lints lms itself with lms::default_linters() (i.e. nested_calls = FALSE), so the new linter won't recursively fire on lms's own source — no self-inflicted CI breakage.
• No .qmd, _quarto*.yml, references.bib, or _site/_freeze changes — no website/build/CI-contract impact.

Minor, non-blocking observation

• The PR description says "Not run: R CMD check." This is disclosed transparently rather than hidden, and the described test coverage (17 expectations, full lms suite passing) is reasonable for a scoped linter addition. Still, per the lab manual's QA checklist, a devtools::check()/R CMD check pass before merge would be good practice to catch any Rd/NAMESPACE consistency issues the generated docs might have introduced — worth running before merge, but not something I can confirm is broken from the diff alone, so I'm not treating it as blocking.

No hallucinated functions, packages, or APIs found: lintr::Linter(), lintr::is_lint_level(), lintr::xml_nodes_to_lints(), xml2::xml_find_all() are all real, already-used APIs in this same package. The PR's cross-references ("lds#1752") are external claims I can't verify from this repo but aren't implausible or required for this diff's correctness.

Verdict

Ready for merge

Structured Review Data (JSON)

{
  "schema_version": "1.1",
  "reviewer": "claude",
  "commit_sha": "f5471d9287022594601e525bc199f9dd9a1b9ebb",
  "verdict": "CLEAN",
  "findings": [],
  "detailed_assessment": "Traced lms/R/ggplot_args_linter.R XPath logic against nested_call_linter.R precedent and all 17 testthat expectations; no defect found in the aes-vs-data argument partition or pipe handling.",
  "holistic_assessment": "Change is scoped to lms/ only, adds no new dependency, matches the documented default_linters rollout convention, and has no qmd, bibliography, or CI workflow impact."
}

Reviewed commit: f5471d9

[github-actions]

💰 Cost: $0.8146 (review) — run

[github-actions]

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

_{2026-11-03 18:57 UTC}