Skip to content

Print mis-matched rows via the ARD print method; add NEWS and coverage - #3

Merged
ddsjoberg merged 5 commits into
malanbos:fix/print-compare-ard-missing-rowsfrom
pharmaverse:review/print-compare-ard-followup
Sep 7, 2026
Merged

ddsjoberg merged 5 commits into
malanbos:fix/print-compare-ard-missing-rowsfrom
pharmaverse:review/print-compare-ard-followup

Conversation

@ddsjoberg

@ddsjoberg ddsjoberg commented Sep 7, 2026 •

Copy link
Copy Markdown

Follow-up to pharmaverse#607, targeting that PR's branch rather than main. The one-line fix in pharmaverse#607 is correct — this adds the pieces the review flagged.

1. Print the mis-matched rows through the ARD print method

compare_ard() landed 2026-05-04 (ee86fef, pharmaverse#552), back when scalar list-columns printed as <chr [1]> under tibble — as.data.frame() was the workaround for that. The pillar/tibble print rewrite landed 2026-08-06 (69c1c00, pharmaverse#594), and card_list_shaft() prints scalar list-column elements as their values now. The coercion has been vestigial since.

The frames are still card objects: .compare_rows() is select() + anti_join(), and dplyr's default dplyr_reconstruct preserves a custom tibble subclass through both (verified on dplyr 1.2.1 — no dplyr_reconstruct.card method is needed or defined). So dropping the coercion gives us:

  • bounded output — #607 as it stands prints the whole frame, so two large ARDs disagreeing on thousands of keys flood the console. tibble prints 10 rows once there are more than 20, with an honest # i N more rows footer. Better than a head(n = 10) that drops rows silently.
  • correct list-column rendering — group1_level / variable_level are list-columns; base print.data.frame() only reached their values via format.default()'s paste(collapse = ", ") fallback.
-- Rows in `x` that do not appear in `y`.
# An ARD data frame: 45 x 3
   variable variable_level stat_name
   <chr>    <list>         <chr>
 1 SITEID   702            n
 ...
10 SITEID   706            n
# i 35 more rows

The comparison tables get the same treatment -- they are card objects too (.compare_columns() builds them with inner_join()), and print(x, n = 10) reports the withheld rows in the footer instead of utils::head() silently dropping them.

dplyr::as_tibble() is deliberately not used: it strips the card class, which sends stat.x/stat.y/difference back to printing as <dbl [1]> instead of their values.

One caveat worth a look: pillar formats to pillar.sigfig (3) significant digits where as.data.frame() used getOption("digits") (7). Values differing below the 3rd significant digit now render alike side by side -- 1.5 vs 1.500001 becomes 1.5 vs 1.50. The difference column still carries the full all.equal() magnitude. Setting pillar.sigfig inside the print method would restore the old precision if you want it.

2. NEWS.md

Added under # cards 0.9.0.9000, with the (#605, @malanbos) attribution, so it isn't a manual step at merge time.

3. Test coverage

  • Both blocks non-empty at once — all three snapshots in Print the rows in x that do not appear in y in print.compare_ard() pharmaverse/cards#607 have at most one of the two blocks populated. Two headers and two frames back-to-back is what a future refactor of this method is most likely to break.
  • A long block truncating — 45 mis-matched rows, which is what backs the NEWS claim.
  • An expect_output() assertion outside of any snapshot. The regression here is "output silently missing", which is exactly what a wholesale snapshot_accept() will swallow. The >80 rows exist in only one of the two ARDs, so they are never part of the comparison tables — seeing >80 on stdout means the mis-matched row block itself printed. cli writes to stderr, so it is not captured.

4. Widen the console in the snapshot tests

testthat pins width to 80, which split the ~120-character comparison tables into two column chunks:

    group1  group1_level variable variable_level stat_name  stat.x
  1    ARM       Placebo   AGEGR1          65-80         N      56
       stat.y                          difference
  1        72 Mean relative difference: 0.2857143

withr::local_options(width = 200) at test scope prints each table in one piece. expect_snapshot() does not re-pin width, so the option survives it.

Test-only on purpose: withr is in Suggests, and in a real session cli falls back to getOption("width"), so widening inside print.compare_ard() would stretch the section rules to match. Under testthat cli.width is pinned separately, so the rules stay 80 characters and the mis-matched row blocks are byte-identical. Net effect is 34 fewer snapshot lines, since unchunking drops the repeated row-index columns — which also takes some of the sting out of the fixture-size point below.

Not done

The review also suggested narrowing the snapshot fixtures (dropping by = "ARM") to cut churn from the incidental stat comparison values. I left them: every other test in test-print.R uses ard_tabulate(ADSL, by = "ARM", variables = "AGEGR1"), and consistency within the file seemed worth more than the ~20 lines. Happy to change it if you disagree.

Verification

Full testthat::test_local() suite passes locally (one pre-existing R-version skip). styler reports no changes on R/print.R or tests/testthat/test-print.R.

🤖 Generated with Claude Code

ddsjoberg and others added 5 commits September 7, 2026 15:36
Follow-up to the #605 fix.

`compare_ard()` landed before ARDs printed through pillar/tibble (#594), so
`print.compare_ard()` coerced with `as.data.frame()` to get scalar list-column
values to show. `card_list_shaft()` does that now, and the frames are still
`card` objects -- dplyr's default `dplyr_reconstruct` preserves the class
through the `select()`/`anti_join()` in `.compare_rows()`. Dropping the
coercion in the two mis-matched row blocks bounds the output (10 rows plus a
`# i N more rows` footer) and renders `group1_level`/`variable_level` properly.

The comparison block keeps `as.data.frame()` + `head(n = 10)`: its `difference`
column holds `all.equal()` messages that pillar would truncate with `~` rather
than wrap.

Also adds the NEWS.md entries, a snapshot covering both mis-matched row blocks
being non-empty at once, a snapshot showing a long block truncating, and an
`expect_output()` assertion outside of any snapshot so that regenerating
snapshots cannot quietly re-accept the missing output from #605.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
testthat pins `width` to 80, which splits the ~120-character comparison
tables into two column chunks and makes the snapshots hard to read. Setting
`withr::local_options(width = 200)` at test scope prints each table in one
piece. `expect_snapshot()` does not re-pin `width`, so the option survives.

`cli.width` is pinned separately by testthat, so the section rules stay 80
characters wide, and the ARD print method is unaffected -- the mis-matched
row blocks are byte-identical. Net effect is 34 fewer snapshot lines, since
unchunking drops the repeated row-index columns.

The option is deliberately test-only: `withr` is in Suggests, and in a real
session `cli` falls back to `getOption("width")`, so widening inside
`print.compare_ard()` would stretch the section rules to match.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`print.compare_ard()` coerced the comparison tables with `as.data.frame()` and
truncated them with `utils::head(n = 10)`. The tables are already `card`
objects -- `.compare_columns()` builds them with `inner_join()`, which
preserves the class -- so printing them directly gives tibble formatting with
the scalar list-column values shown, and `n = 10` reports the withheld rows in
the footer instead of silently dropping them.

`dplyr::as_tibble()` is not used here: it strips the `card` class, which sends
the `stat.x`/`stat.y`/`difference` list-columns back to printing as
`<dbl [1]>` rather than their values.

Note that pillar formats numbers to `pillar.sigfig` (3) significant digits,
where `as.data.frame()` used `getOption("digits")` (7), so near-identical
values now render alike in the side-by-side columns. The `difference` column
still carries the full `all.equal()` magnitude.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cli's builtin theme gives `h3` a `margin-top` of 1, so every
"Rows in `x`/`y` that do not appear in ..." sub-header was preceded by a blank
line -- both under the "Mis-matched Rows" rule and between the two row blocks.
A `cli_div()` theme zeroes it; the div closes itself when the method exits.

`h1` keeps its `margin-top`, so the "Mis-matched Rows" and "Comparison Results"
rules still separate from what precedes them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The hard-coded `n = 10` came from the `utils::head(n = 10)` it replaced, but
it diverges from the mis-matched row blocks, which use a plain `print()`, and
it overrides whatever the user set `tibble.print_min`/`tibble.print_max` to.

The default prints every row up to `tibble.print_max` (20) and truncates to
`tibble.print_min` (10) beyond that, so long tables are still bounded -- the
45-row block in the truncation test still reports `# i 35 more rows`. The only
change is that the 12-row comparison tables now print in full.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ddsjoberg
ddsjoberg merged commit 0fc9567 into malanbos:fix/print-compare-ard-missing-rows Sep 7, 2026
1 check failed
@ddsjoberg
ddsjoberg deleted the review/print-compare-ard-followup branch September 7, 2026 23:14
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant