Excel leap day bug

[billdenney]

29 Feb 1900 is not a real date (it does not exist on real calendars), but it does exist in Excel. This makes it a warning to have an Excel numeric date of 29 Feb 1900 and converts the value to NA. (Applied on top of #421 since there is an interaction.)

[codecov]

Codecov Report

Merging #422 (1e60a7b) into master (6f4a800) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #422   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          950       958    +8     
=========================================
+ Hits           950       958    +8     

Impacted Files	Coverage Δ
R/excel_dates.R	100.00% <100.00%> (ø)

[billdenney]

Hmm, Windows and Mac work while Linux does not. This will take some investigation.

[billdenney]

Fix #420

[sfirke]

Looks good to me, thank you for fixing both of these! Edge cases to be sure, but also definitely incorrect behavior (thanks Microsoft). I will see if I can poke at the Linux test failure and get anywhere.

[sfirke]

Indeed, those tests fail for me on my Ubuntu machine. This works, when it shouldn't:

> excel_numeric_to_date(43170.09, include_time=TRUE, tz="America/New_York")
[1] "2018-03-11 03:09:36 EDT"

And this errors before the comparison it is part of can be executed;

>     as.POSIXct(NA_real_, tz="America/New_York")
Error in as.POSIXct.numeric(NA_real_, tz = "America/New_York") : 
  'origin' must be supplied

[billdenney]

I think that this is a subtle bug in as.POSIXct.numeric() where all NA inputs should return NA rather than requiring the origin. (I'm kinda surprised that we discovered a base R bug...) We should probably just add an origin argument for now, and report it. I'll try those out and push a change.

[billdenney]

I found a few issues with as.POSIXct and as.POSIXlt while working through this. A simpler solution may be to add lubridate as a dependency and use its date creation which seems to work correctly for daylight savings times.

What do you think about a lubridate dependency?

[sfirke]

Relying on lubridate to sweat the details around dates sounds wise to me! 🦉 No worries about adding that dependency, virtually everyone using this package will have it installed as part of tidyverse anyway.

[billdenney]

It turns out that we already had lubridate as a dependency due to convert_to_date() and convert_to_datetime(). Also, I brought up the as.POSIX issues here: https://stat.ethz.ch/pipermail/r-help/2021-January/469943.html.

[sfirke]

Looks great, thank you for digging in to sort this out so carefully 🙏

[sfirke]

I think this closes #420, is that right?

[billdenney]

It does.