integer64 support (follow up to #121) #122
Conversation
romainfrancois
changed the title
|
Is it worth doing this as a record with a single field for better control of things and then relay back to bit64 only for the low level ? |
|
I'd prefer to stick with raw bit64 for now |
| @@ -27,7 +27,8 @@ Suggests: | |||
| pkgdown, | |||
| rmarkdown, | |||
| testthat, | |||
| tibble | |||
| tibble, | |||
| bit64 | |||
There was a problem hiding this comment.
Can we really add bit64 as suggest but not under imports? In arrow, it was easy to hit crashes if bit64 gets unloaded, see apache/arrow#2867, but maybe that's not the case in vctrs, just passing along FWIW.
There was a problem hiding this comment.
I think it's fine to use Suggests and then bit64:: when we need a function from it.
vec_type2(<int64>, NA) -> <int64> vec_type2(NA, <int64>) -> <int64>
|
Was there anything else needed for this ? |
|
I just need some time to think it through. Is it blocking you? |
|
Not really, https://github.com/apache/arrow/blob/master/r/DESCRIPTION#L31 |
There was a problem hiding this comment.
Can you please also add a regression test like https://github.com/r-lib/vctrs/blob/master/tests/testthat/test-type-date-time.R#L16-L34
R/type-integer64.R
Outdated
| #' @param ...,class Used to for subclasses. | ||
| #' @keywords internal | ||
| #' @export | ||
| new_int64 <- function(x = bit64::integer64(), ..., class = character()) { |
There was a problem hiding this comment.
Do we actually need this function?
There was a problem hiding this comment.
Not really, was mostly following the pattern, and provision for derived classes.
|
|
||
| # Coerce ------------------------------------------------------------------ | ||
|
|
||
| #' @export |
There was a problem hiding this comment.
Can you please check this is up to date with the advice in https://vctrs.r-lib.org/articles/s3-vector.html#double-dispatch?
There was a problem hiding this comment.
I'm just unsure about
#' @export
#' @method vec_cast.integer64 logical
vec_cast.integer64.logical <- function(x, to) {
bit64::as.integer64(x)
}Does this need to distinguish the unspecified case given that cast to logical is allowed, or is this good enough:
library(vctrs)
x <- bit64::as.integer64(1:10)
vec_cast(x, unspecified())
#> Error: Can't cast <integer64> to <vctrs_unspecified>
vec_cast(x, NA)
#> [1] TRUE TRUE TRUE TRUE TRUE TRUE TRUE TRUE TRUE TRUECreated on 2018-11-20 by the reprex package (v0.2.1.9000)
There was a problem hiding this comment.
It's the other way around:
library(vctrs)
x <- bit64::as.integer64(1:10)
vec_cast(unspecified(), x)
vec_cast(NA, x)And I think you need to use vec_unspecified_cast(x, to) in that method
- new_int64 function + vec_type2.integer64.vctrs_unspecified + vec_type2.vctrs_unspecified.integer64
|
Looks good. I'll merge once vctrs is on CRAN. |
|
@hadley can we merge since |
|
Done! |
No description provided.