Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix db_get_zoom() to support zooming on empty tables #628

Merged
merged 2 commits into from
Sep 27, 2021

Conversation

moodymudskipper
Copy link
Collaborator

@moodymudskipper moodymudskipper commented Sep 27, 2021

closes #626

This is a small fix for a very interesting error!
In fact the error we see in linked issue is not one, it's just a message, and it's sent by RStudio, not base R.

Here's a minimal reproduction of the issue :

length.foo = function(x) stop("error!")
registerS3method("length", "foo", length.foo)
foo <- 1
attributes(foo) <- list(class = "foo")
#> Error in length.foo(obj) : error!
foo
#> [1] 1
#> attr(,"class")
#> [1] "foo"

As we see, the fooobject was properly created and modified, but the fact that the length method fails makes RStudio display the problematic message.

The guilty function is .rs.describeObject, it seems to be called after an object is created or modified in the console, and it calls length().

In our case we have length() --> length.zoomed_dm() --> tbl_zoomed() --> dm_get_zoom(), and dm_get_zoom(our_zoomed_dm) fails through abort_not_pulling_multiple_zoomed()

Our code didn't call length() on our object, so the bug is not our fault but
Rstudio's, however this showed us that our length method has a problem since
a zoomed dm containing an empty table is legal but its length isn't.

The idiom lengths(zoom) != 0 was used to filter the zoomed rows, likely expecting zero length elements to be NULL, but length(tibble()) has length zero too. We use !map_lgl(zoom, is.null) instead and the issue disappears.

I opened a RStudio issue too : rstudio/rstudio#9887

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

R/dm.R Outdated Show resolved Hide resolved
Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com>
Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@krlmlr krlmlr merged commit 6ebe54d into main Sep 27, 2021
@krlmlr krlmlr deleted the b-626-error-zooming-on-empty-table branch September 27, 2021 17:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Weird error when zooming into an empty table
2 participants