ci+fix: strict lifecycle deprecations, idiom review guidance, bot-skip review#244
Merged
Conversation
…p review Make non-idiomatic / soon-to-be-removed tidyverse usage fail loudly and steer reviewers (human + AI) to flag it, prompted by a data-masking-in- tidyselect issue that slipped through every review on PR #240. - tests/testthat/setup.R: set options(lifecycle_verbosity = "error") so lifecycle deprecations -- including *soft* ones like the .data pronoun in a tidy-selection context, which never warn in an installed-package test run -- fail the suite instead of passing silently. - R/calc_fit_mod.R: convert the four tidyselect-context .data uses (select(), .by, pivot_wider names_from/values_from) to all_of()/string selection so the suite is green under the stricter option. Data-masking .data uses in summarize()/mutate() expressions are left as-is. No change to behavior or output. (Collides with the concurrent rewrite on #240; Sam to resolve conflicts on that branch.) - .github/workflows/claude-code-review.yml: skip (don't fail) the review job when github.event.sender.type == 'Bot' (or actor is *[bot]'), so a commit pushed by @claude or Copilot no longer red-Xes a review. - CLAUDE.md (new) + .github/copilot-instructions.md: add an idiomatic-code review focus -- flag data-masking in tidyselect, if/else that only varies columns, base merge() over dplyr joins -- citing the lab-manual coding-style chapter as the canonical authority. - NEWS.md / DESCRIPTION / inst/WORDLIST: changelog, version bump, wordlist. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
|
R CMD check (run with error_on = "note") flagged CLAUDE.md as a non-standard top-level file, failing all platforms. Add it to .Rbuildignore so it ships as repo guidance only, not package contents. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
On #240, a reviewer (d-morrison) caught
.datadata-masking used in a tidy-selection context inR/calc_fit_mod.R— and none of the AI review workflows flagged it. Root cause: it's a soft deprecation (silent in installed-package test runs, sooptions(warn = 2)never catches it), and the@claude/Copilot reviewers were optimizing for "does the fix work," not idiom. This PR closes both gaps and cleans up the existing offenders.What
tests/testthat/setup.Rsetsoptions(lifecycle_verbosity = "error"), which overrides soft-deprecation suppression so lifecycle deprecations (incl..data-in-tidyselect) fail the suite. (Scope: all test runs.)calc_fit_mod()— the only file onmainusing.datain tidyselect contexts. Convertedselect(),.by, andpivot_wider(names_from/values_from)toall_of()/ string selection. Data-masking.data$insummarize()/mutate()expressions left as-is. No behavior/output change.claude-code-review.ymlgains a job-levelif:that skips whengithub.event.sender.type == 'Bot'(or actor is*[bot]). Ports the pattern already used ingdl/asaurl/bcs/rme/serocalculator.mainis not branch-protected, so a skipped check doesn't block merge.CLAUDE.md(the file@claudelooks for) + an expanded Code Style / "Review focus" section in.github/copilot-instructions.md. Directs reviewers to flag data-masking-in-tidyselect,if/elsethat only varies columns, and basemerge()over dplyr joins, citing the lab-manual coding-style chapter.Validation
spelling::spell_check_package()→ no errors (addeddeprecations,lifecycleto WORDLIST).lintr::lint()oncalc_fit_mod.Randsetup.R→ no lints.lifecycle_verbosity = "error": the newall_of()forms pass; the old.data-in-select()form errors — confirming both that the option catches the issue and that the fix resolves it.Note on #240
This touches
R/calc_fit_mod.R, which #240 is concurrently rewriting. Per discussion, @sschildhauer to resolve the merge conflict on the #240 branch after this lands. #240's idiomatic rewrite (theany_of()+ join restructuring from the inline review) supersedes this minimal de-deprecation.🤖 Generated with Claude Code