feat: `as_tibble()` calls `as.data.frame()` for objects that are not subclasses of `"tbl_df"` #1557

TimTaylor · 2023-10-27T10:27:50Z

This patch ensures as_tibble.data.frame first calls as.data.frame
to allow for methods provided by extended data frame classes. The
idea was proposed by Jan Gorecki in
Rdatatable/data.table#5698.

Fixes #1556.

TimTaylor · 2023-10-27T10:44:45Z

Failing on pre 4.0.0 release due to use of .S3method() which is "new". Unsure if this is an issue for you or not as the usage is only within the test, not package, code.

krlmlr

Thanks! Running revdepchecks now.

krlmlr · 2023-10-27T10:44:44Z

tests/testthat/test-as_tibble.R

+ x <- structure(mtcars, extra = "extra", class = c("ext_df_", "data.frame"))
+ y <- as_tibble(head(mtcars))
+ fun <- function(x, row.names = NULL, optional = FALSE, ...) y
+ .S3method("as.data.frame", "ext_df_", fun)


We have local_bindings() for this: https://github.com/r-lib/waldo/blob/56819ce69801b4ff93f4c62e5572bf7bfdd4db4a/tests/testthat/test-compare.R#L390 .

Now updated using with_mocked_bindings().

krlmlr · 2023-10-27T10:45:42Z

tests/testthat/test-as_tibble.R

+test_that("as_tibble.data.frame coerces extended data.frames first", {
+ x <- structure(mtcars, extra = "extra", class = c("ext_df_", "data.frame"))
+ y <- as_tibble(head(mtcars))
+ fun <- function(x, row.names = NULL, optional = FALSE, ...) y


I wonder if this method should be doing something more exciting to verify that it's actually called. Maybe set a flag in the test, or remove some attribute of the object?

I thought it was pretty extreme as is (returning head of global variable with one less attribute) but I may be misunderstanding

krlmlr · 2023-10-27T13:38:14Z

I suspect it's worth understanding the dplyr failure -- the other failures could be a consequence of that.

TimTaylor · 2023-10-27T15:16:42Z

I suspect it's worth understanding the dplyr failure -- the other failures could be a consequence of that.

hmm - I hadn't seen that there was no explicit method for tbl_df.

Now fixed (hopefully) with latest commit. I've rechecked dplyr and seems ok so 🤞 . Note the actions are currently failing but not due to package code.

krlmlr · 2023-10-27T15:30:52Z

Perhaps we want a separate as_tibble.tbl_df() method that would do less work? I vaguely remember this existed at some point.

It's always amazing to see how an innocuous change can quickly turn into a deep rabbit hole. I can't spend too much time digging myself, but happy to assist.

I'm seeing the GHA failure elsewhere, need to fix it.

TimTaylor · 2023-10-27T15:37:21Z

Yeah - I wondered that too but didn't want to make the hole any bigger. I think one or two other packages (e.g. LexisNexisTools) may be genuine breakages (only due to an unneeded attribute that has been tested against). I'll dig in to these over the next few days and update with any info. No rush to get this sorted - we will get there in the end.

TimTaylor · 2023-11-02T09:04:27Z

@krlmlr - I've looked through the revdep problems in more detail and I'm pretty sure it is only {LexisNexisTools} that will need patching. Is it possible to bump the actions and then if possible your revdep checks? (tests failed after 23dd204 due to the r-project domain issue).

krlmlr · 2023-12-17T19:22:27Z

I wonder what's wrong with the GHA checks. The main branch looks good.

TimTaylor · 2023-12-19T16:12:22Z

Happy to poke at the revdeps again if it's not too costly and they can be rerun? (unsure on your CI setup for the gazillion revdeps you have).

This patch ensures as_tibble.data.frame first calls as.data.frame to allow for methods provided by extended data frame classes. The idea was proposed by Jan Gorecki in Rdatatable/data.table#5698.

krlmlr · 2023-12-19T16:20:18Z

Thanks. I think it's good as is, I'll wait for a bit until the CRAN release.

jangorecki · 2023-12-19T16:27:42Z

tests/testthat/test-as_tibble.R

+ y <- as_tibble(head(mtcars))
+ with_mocked_bindings(
+ as.data.frame = function(x, row.names = NULL, optional = FALSE, ...) y,
+ code = expect_identical(as_tibble(x), y),


Just from looking at the code here it feels it should fail because y is only a head of mtcars while x is complete mtcars.

Why should it fail? Are you expecting as_tibble(x) to be identical to mtcars converted to a tibble?

Would the test would be clearer if we used two completely unrelated data frames for x and y?

Would the test would be clearer if we used two completely unrelated data frames for x and y?

It's been a while since I wrote the test and it did take me a moment to grok what was going on when rereading so it may be worth changing to y <- airquality . I can also add some commentary and/or link to the issue/PR for context/clarity if useful.

because y was created from already filtered out object, using head(mtcars) 3 lines above while x contains complete mtcars

But the tests succeed, and the example should also work when running it line by line.

krlmlr reviewed Oct 27, 2023

View reviewed changes

krlmlr force-pushed the patch-as_tibble branch from 27f40c1 to 23dd204 Compare October 27, 2023 15:35

TimTaylor and others added 6 commits December 19, 2023 17:18

Fixes tidyverse#1556

990d733

This patch ensures as_tibble.data.frame first calls as.data.frame to allow for methods provided by extended data frame classes. The idea was proposed by Jan Gorecki in Rdatatable/data.table#5698.

use mocked bindings for test

a36c56d

revdepcheck results

0f15e12

account for tibbles as well

7550b5a

Update results

b5058b8

Move past deprecate_stop() call

9a6911b

krlmlr force-pushed the patch-as_tibble branch from 2e64850 to 9a6911b Compare December 19, 2023 16:18

krlmlr changed the title ~~Fixes #1556~~ feat: as_tibble() calls as.data.frame() for objects that are not subclasses of "tbl_df" Dec 19, 2023

krlmlr merged commit 3c4744a into tidyverse:main Dec 19, 2023
3 checks passed

jangorecki reviewed Dec 19, 2023

View reviewed changes

krlmlr mentioned this pull request Apr 15, 2024

Compatibility with the development version of tibble JBGruber/LexisNexisTools#30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: `as_tibble()` calls `as.data.frame()` for objects that are not subclasses of `"tbl_df"` #1557

feat: `as_tibble()` calls `as.data.frame()` for objects that are not subclasses of `"tbl_df"` #1557

TimTaylor commented Oct 27, 2023 •

edited by krlmlr

Loading

TimTaylor commented Oct 27, 2023 •

edited

Loading

krlmlr left a comment

krlmlr Oct 27, 2023

TimTaylor Oct 27, 2023

krlmlr Oct 27, 2023

TimTaylor Oct 27, 2023 •

edited

Loading

krlmlr commented Oct 27, 2023

TimTaylor commented Oct 27, 2023 •

edited

Loading

krlmlr commented Oct 27, 2023

TimTaylor commented Oct 27, 2023

TimTaylor commented Nov 2, 2023

krlmlr commented Dec 17, 2023

TimTaylor commented Dec 19, 2023

krlmlr commented Dec 19, 2023

jangorecki Dec 19, 2023

krlmlr Dec 19, 2023

TimTaylor Dec 19, 2023 •

edited

Loading

jangorecki Dec 19, 2023 •

edited

Loading

krlmlr Dec 21, 2023

feat: as_tibble() calls as.data.frame() for objects that are not subclasses of "tbl_df" #1557

feat: as_tibble() calls as.data.frame() for objects that are not subclasses of "tbl_df" #1557

Conversation

TimTaylor commented Oct 27, 2023 • edited by krlmlr Loading

TimTaylor commented Oct 27, 2023 • edited Loading

krlmlr left a comment

Choose a reason for hiding this comment

krlmlr Oct 27, 2023

Choose a reason for hiding this comment

TimTaylor Oct 27, 2023

Choose a reason for hiding this comment

krlmlr Oct 27, 2023

Choose a reason for hiding this comment

TimTaylor Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

krlmlr commented Oct 27, 2023

TimTaylor commented Oct 27, 2023 • edited Loading

krlmlr commented Oct 27, 2023

TimTaylor commented Oct 27, 2023

TimTaylor commented Nov 2, 2023

krlmlr commented Dec 17, 2023

TimTaylor commented Dec 19, 2023

krlmlr commented Dec 19, 2023

jangorecki Dec 19, 2023

Choose a reason for hiding this comment

krlmlr Dec 19, 2023

Choose a reason for hiding this comment

TimTaylor Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

jangorecki Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

krlmlr Dec 21, 2023

Choose a reason for hiding this comment

feat: `as_tibble()` calls `as.data.frame()` for objects that are not subclasses of `"tbl_df"` #1557

feat: `as_tibble()` calls `as.data.frame()` for objects that are not subclasses of `"tbl_df"` #1557

TimTaylor commented Oct 27, 2023 •

edited by krlmlr

Loading

TimTaylor commented Oct 27, 2023 •

edited

Loading

TimTaylor Oct 27, 2023 •

edited

Loading

TimTaylor commented Oct 27, 2023 •

edited

Loading

TimTaylor Dec 19, 2023 •

edited

Loading

jangorecki Dec 19, 2023 •

edited

Loading