Only na_if() double or integer columns

[DavisVaughan]

This PR makes your package compatible with the next version of dplyr:

na_if() now uses vctrs. It also now casts y to the type of x to ensure that it is type stable on x (the primary input). This means it is now a little stricter. In your case you were calling na_if(x, Inf) and x was possibly a double, Date, factor, or character, and really only the first of those was ever supposed to work. I've worked around this by creating a wrapper that skips the na_if() if the input isn't a double or integer vector.

We plan to submit dplyr 1.1.0 on January 27th.

This should be compatible with both dev and CRAN dplyr. It would help us out if you could go ahead and send a patch version of your package to CRAN ahead of time! Thanks!

[davidhodge931]

Thanks @DavisVaughan

Not sure I understand your utils.R function. Specifically why does it need the is.object condition, and why does it need to convert integers to doubles?

na_if_double <- function(x) {
  if (is.object(x)) {
    return(x)
  }
  
  if (is.integer(x)) {
    x <- as.double(x)
  }
  
  if (is.numeric(x)) {
    x <- dplyr::na_if(x, Inf)
  }
  
  x
}

Can I not just do this?

na_if_double <- function(x) {
  
  if (is.integer(x) | is.double(x)) {
    x <- dplyr::na_if(x, Inf)
  }
  
  x
}

Or maybe I am better to use ifelse, if dplyr::na_if is not definitely going to be around in the long term?

na_if_double <- function(x) {
  
  if (is.integer(x) | is.double(x)) {
    x <- ifelse(is.infinite(x), NA, x)
  }
  
  x
}

[DavisVaughan]

is.integer(x) | is.double(x) will return TRUE for Dates, and you can't cast a double Inf to a Date using vctrs, so the is.object() check is needed to avoid that

[DavisVaughan]

It only needs to convert integers to double if you have an integer input to this function, and it kind of looked like that would be the case for you.

You can't coerce a double Inf value to an integer, there is no equivalent integer value for that.

[davidhodge931]

Great, thanks @DavisVaughan

One last question: does it matter whether return is used or not?

[DavisVaughan]

You need return() if you are doing an early return like I was there

[davidhodge931]

Thanks for this @DavisVaughan just submitted patch to CRAN :)