Guard against loss of class #567
Conversation
Avoid potential dispatch of a `[.spec_tbl_df` method that prematurely drops the subclass and spec and problems attributes
| } | ||
|
|
||
| vroom_select <- function(x, col_select, id) { | ||
| spec <- attr(x, "spec") |
There was a problem hiding this comment.
The need to store and restore attributes comes from the possibility that a call to x[whatever] could drop attributes, depending on the S3 dispatch for [. If we need to preserve spec then we also need preserve problems, i.e. they should get the same treatment, whatever that may be.
In the current solution, x won't have any "special" subclasses yet and [ should preserve attributes.
| } | ||
|
|
||
| out <- tibble::as_tibble(out, .name_repair = identity) | ||
| class(out) <- c("spec_tbl_df", class(out)) |
There was a problem hiding this comment.
Applying "spec_tbl_df" class after calling vroom_select() is the main fix here.
|
The CI failure on macOS is due to some weirdness with the runner, I think. I've been seeing it intermittently for the past couple of days and seems to come from a lockfile being read too soon after it's been written. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the "spec_tbl_df" class and associated attributes (spec and problems) were being lost when using col_select with readr attached. The fix reorders operations to apply the class after column selection instead of before.
Key changes:
- Move
"spec_tbl_df"class assignment to aftervroom_select()call invroom() - Remove manual
specattribute preservation logic invroom_select()since it's no longer needed - Add comprehensive tests for both
vroom()andvroom_fwf()to verify the fix
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| R/vroom.R | Moves "spec_tbl_df" class assignment to after vroom_select() to prevent loss via readr's [.spec_tbl_df method |
| R/col_types.R | Removes now-unnecessary manual spec attribute preservation in vroom_select() |
| tests/testthat/test-vroom.R | Adds test verifying vroom() preserves class and attributes when readr's [.spec_tbl_df is registered |
| tests/testthat/test-vroom_fwf.R | Adds test verifying vroom_fwf() preserves class and attributes when readr's [.spec_tbl_df is registered |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes #554, fixes #534, retroactively provides an alternative fix for #303 (#566 cleared the way for nicer fixes here)
The problem is that
vroom()was applying the"spec_tbl_df"class before callingvroom_select(), which uses[internally. If readr is attached, this dispatches readr's[.spec_tbl_dfmethod, which is designed to drop the"spec_tbl_df"class and thespecandproblemsattributes. The target use case is for downstream user manipulation of a tibble that was originally parsed by readr or vroom. But it's basically a mistake to use this[.spec_tbl_dfwhile you're still in the process of creating the tibble.(Side note: the order of operations was correct in
vroom_fwf()already.)One thing this has brought up for me is ... why doesn't vroom have a
[.spec_tbl_dfmethod of its own? 🤔 Seems like the same considerations would apply, which is that, once a user starts to subset a vroom-produced tibble, you'd want to ditchspecandproblemsand the subclass (the original motiavtion is spelled out in tidyverse/readr@e94b8bc). vroom conditionally registers practically every other S3 method that readr does. Why not this one? Recorded this question in #569.