Remove arrangements for older versions of R that vroom does not support #595
Conversation
jennybc
changed the title
jennybc
changed the title
| which$int %||% vroom_use_altrep_int(), | ||
| which$dbl %||% vroom_use_altrep_dbl(), | ||
| which$num %||% vroom_use_altrep_num(), | ||
| which$lgl %||% vroom_use_altrep_lgl(), |
There was a problem hiding this comment.
vroom actually does not ever use altrep for logical columns, but I gather there must have been some thought that it might one day? Anyway, I'm not getting into that here.
There was a problem hiding this comment.
Probably the fact that it didn't exist in 3.5 delayed its implementation, and then it was never added later on?
There was a problem hiding this comment.
Yeah? But I also think the performance argument is so different (less compelling) for logical. I'd at least want to look into that quantitatively before ever considering adding that feature.
| // if (collector.use_altrep()) { | ||
| //#if defined HAS_ALTREP && R_VERSION >= R_Version(3, 6, 0) | ||
| // SET_VECTOR_ELT(res, i, vroom_lgl::Make(info)); | ||
| //#endif | ||
| //} else { |
There was a problem hiding this comment.
More evidence of vague-but-unrealized plans around altrep logical columns.
| ## RStudio caveats | ||
|
|
||
| RStudio’s environment pane calls `object.size()` when it refreshes the | ||
| pane, which for Altrep objects can be extremely slow. RStudio 1.2.1335+ | ||
| includes the fixes | ||
| ([RStudio#4210](https://github.com/rstudio/rstudio/pull/4210), | ||
| [RStudio#4292](https://github.com/rstudio/rstudio/pull/4292)) for this | ||
| issue, so it is recommended you use at least that version. | ||
|
|
There was a problem hiding this comment.
This now feels like ancient history that need not be mentioned.
| #undef FALSE | ||
| #include <cpp11/logicals.hpp> | ||
|
|
||
| #ifdef VROOM_USE_CONNECTIONS_API |
There was a problem hiding this comment.
The connections API never happened (or un-happened, I guess), so doing some cleanup around that too.
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
Nice, i did a similar purge in vctrs recently. Very cathartic!
| which$int %||% vroom_use_altrep_int(), | ||
| which$dbl %||% vroom_use_altrep_dbl(), | ||
| which$num %||% vroom_use_altrep_num(), | ||
| which$lgl %||% vroom_use_altrep_lgl(), |
There was a problem hiding this comment.
Probably the fact that it didn't exist in 3.5 delayed its implementation, and then it was never added later on?
Closes #594
@DavisVaughan I'm asking for review just to get a second set of eyeballs on this and make sure I'm not missing any obvious downside. I feel like this is a net positive for the codebase.
More context re: why I want to clean up dusty bits: I have a medium term goal of moving the guts of vroom into readr (readr really is vroom at this point, for the vast majority of usage) and thereby eliminating lots of annoyance around maintenance, documentation, error messaging, name collisions, etc. At the same time, standalone vroom would be put into a frozen state and on a path to eventual archival.