-
Notifications
You must be signed in to change notification settings - Fork 20
Fix up formatting for S3 objects #916
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
Conversation
@@ -65,6 +65,7 @@ jobs: | |||
tibble | |||
haven | |||
R6 | |||
survival |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added survival here, despite it being (because it is?) such a problem child so we can write tests for this especially weird/bad case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it's failing is that we are nor dispatching on length, so we are seeing length = 10
when formatting.
We should likely dispatch on the S3 implementation of length here:
ark/crates/harp/src/vector/formatted_vector.rs
Lines 63 to 64 in 1bed8eb
let length = r_length(self.vector.sexp); | |
let n = n.min(length as usize); |
and
ark/crates/harp/src/vector/formatted_vector.rs
Lines 72 to 73 in 1bed8eb
let length = r_length(self.vector.sexp); | |
Another possibibily is to implement a custom display method for Surv objects:
ark/doc/variables-pane-extending.md
Lines 11 to 25 in 1bed8eb
```r | |
ark_positron_variable_display_value.foo <- function(x, ..., width = NULL) { | |
toString(x, width) | |
} | |
``` | |
These methods don't need to be exported in the `NAMESPACE` file. Ark automatically finds and registers them when the package is loaded. | |
You can also register a method outside an R package using `.ps.register_ark_method()`, similar to `base::.S3method()`: | |
```r | |
.ps.register_ark_method("ark_positron_variable_display_value", "foo", function(x, width) { | |
toString(x, width) | |
}) | |
``` |
Although, I think it should be fine to dispatch on length()
. We may find some other commonly used S3 object that doesn't correctly report it's length and could cause different problems.
Thanks, @dfalbel!
I tend to think it might be better to use |
2161a4a
to
3c6271e
Compare
Amazing, @dfalbel, thank you so much! Release NotesNew Features
Bug Fixes
QA NotesFor code like this: library(tidyverse)
library(survival)
res <- modeldata::cat_adoption |>
slice_sample(n = 15) |>
mutate(
event_time = Surv(time, event),
.keep = "unused",
.before = everything()
) You can:
![]() |
Addresses posit-dev/positron#8053
With this PR, you can see
Surv
objects in the Data Explorer:And we no longer get this error I point out here, when you click to expand a
Surv
object in the Variables Pane:Good news on both fronts! 🎉
This manhandling of
dim()
was introduced for posit-dev/positron#1862, also motivated bySurv
objects. However, I think we must have updated other pieces that handle this, because those original examples are fine even after I remove this code:No panics! More good news 🎉
There is still a little bit of bad news. Now when you click to expand a vector of
Surv
objects in the Variables pane, there is a new error:Try with this code:
And the error message has:
Notice that it is trying to subset
6:10
here, which doesn't exist. I suspect this is related to #646, and how we changed to subsetting before formatting.@dfalbel could you take a look at this and see if you have suggested changes we could make here to make this work in the Variables pane? It would be nice to just get it all fixed and write some tests to make sure it does not regress.