Skip to content

Evaluate compare_ard() keys and columns against both ARDs - #608

Open
malanbos wants to merge 1 commit into
pharmaverse:mainfrom
malanbos:fix/compare-ard-select-each-ard
Open

Evaluate compare_ard() keys and columns against both ARDs#608
malanbos wants to merge 1 commit into
pharmaverse:mainfrom
malanbos:fix/compare-ard-select-each-ard

Conversation

@malanbos

@malanbos malanbos commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

What changes are proposed in this pull request?

  • Bug fix in compare_ard() where the keys and columns selections were resolved against x alone and the resulting column positions re-used on y. Each selection is now evaluated against both ARDs, so ARDs holding the same columns in a different order can be compared, and selecting functions may be combined with column names, e.g. keys = c(all_ard_groups("levels"), "stat_name"). (compare_ard() column resolving #606, @malanbos)

.process_keys_arg() and .process_compare_arg() evaluated the tidyselect expression twice. The first evaluation forced the argument, so the second call received the column positions selected from x rather than the expression. Defusing the selection first, with enquo(), means cards_select() receives a quosure on both calls and the selection is made against each ARD in turn. arg_name is now passed through as well, so an unusable selection names the argument it came from.

cards_select() itself is deliberately untouched. process_selectors.R already calls it with defused quosures (expr = x from enquos(...), and expr = lhs_quo), so capturing inside cards_select() would double-capture those.

Tests are added for both symptoms — comparing two ARDs whose columns are in a different order, and keys/columns combining selecting functions with column names. Both fail on main (The keys columns from x and y do not match, and Columns `2` and `3` don't exist) and pass here. The full devtools::test() suite passes, including process_selectors and selectors.

I have left NEWS.md alone — the bullet above is ready to copy in at merge time.

Reference GitHub issue associated with pull request.

Closes #606


Pre-review Checklist (if item does not apply, mark is as complete)

  • All GitHub Action workflows pass with a ✅
  • PR branch has pulled the most recent updates from master branch: usethis::pr_merge_main()
  • If a bug was fixed, a unit test was added.
  • Code coverage is suitable for any new functions/features (generally, 100% coverage for new code): devtools::test_coverage()
  • Request a reviewer

Reviewer Checklist (if item does not apply, mark is as complete)

  • If a bug was fixed, a unit test was added.
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Code coverage is suitable for any new functions/features: devtools::test_coverage()

When the branch is ready to be merged:

  • Update NEWS.md with the changes from this pull request under the heading "# cards (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
  • All GitHub Action workflows pass with a ✅
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge" or "Rebase and merge".
.process_keys_arg() and .process_compare_arg() evaluated the tidyselect
expression twice, which forced the argument on the first call, so the
second call received the column positions selected from x rather than the
expression. Defusing the selection first evaluates it against each ARD in
turn, and lets tidyselect handle selectors combined with column names.
@malanbos
malanbos requested a review from ddsjoberg August 24, 2026 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant