tidyverse / lubridate

Make working with dates in R just that little bit easier
https://lubridate.tidyverse.org
GNU General Public License v3.0
728 stars 207 forks source link

Period and Interval vctrs proxy methods need to tweak handling of names #1072

Closed DavisVaughan closed 1 year ago

DavisVaughan commented 1 year ago

The fact that rcrd_names is an optional column for the vec_proxy() method poses a problem for combination functions

x <- `names<-`(period(1), "x")
y <- period(2)

vctrs::vec_c(x, y)
#> Error in `vctrs::vec_c()`:
#> ! Can't assign 7 columns to df of length 6.
#> ℹ In file 'slice-assign.c' at line 341.
#> ℹ This is an internal error that was detected in the vctrs package.
#>   Please report it at <https://github.com/r-lib/vctrs/issues> with a reprex (<https://https://tidyverse.org/help/>) and the full backtrace.

#> Backtrace:
#>     ▆
#>  1. ├─vctrs::vec_c(x, y)
#>  2. └─rlang:::stop_internal_c_lib(...)
#>  3.   └─rlang::abort(message, call = call, .internal = TRUE, .frame = frame)

x <- `names<-`(interval(.Date(0), .Date(2)), "x")
y <- interval(.Date(0), .Date(3))

vctrs::vec_c(x, y)
#> Error in `vctrs::vec_c()`:
#> ! Can't assign 3 columns to df of length 2.
#> ℹ In file 'slice-assign.c' at line 341.
#> ℹ This is an internal error that was detected in the vctrs package.
#>   Please report it at <https://github.com/r-lib/vctrs/issues> with a reprex (<https://https://tidyverse.org/help/>) and the full backtrace.

#> Backtrace:
#>     ▆
#>  1. ├─vctrs::vec_c(x, y)
#>  2. └─rlang:::stop_internal_c_lib(...)
#>  3.   └─rlang::abort(message, call = call, .internal = TRUE, .frame = frame)

I think the solution is actually pretty easy. Since these are S4 objects it looks like the names are already being put on @.Data, so we may as well leave them there when we promote to a data frame to generate the proxy. The names will get sliced accordingly and then will be restored automatically. This is similar to how it works in clock now.

vspinu commented 1 year ago

@DavisVaughan would you be able to provide a PR for this?

The names are there on @.Data indeed.