Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Account for leap year bug; fixes #264 #292

Merged
merged 2 commits into from
Mar 13, 2017
Merged

Account for leap year bug; fixes #264 #292

merged 2 commits into from
Mar 13, 2017

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Mar 13, 2017

Excel reproduces a bug from Lotus 1-2-3: the non-existent leap day February 29, 1900 is included in the date system. This only affects sheets that use the 1900 date system and dates prior to March 1, 1900.

The proposed fix effectively removes that date. I adjust (increment) date-times on or before February 28, 1900. I convert date-times on February 29 1900 -- a day which does not actually exist -- to NA and throw a warning.

Since knowledge of the date system is now required for both the leap year adjustment and the offset, I store and pass the 1900 vs 1904 indicator around instead of the offset.

Fixes #264 and #148 (already closed as duplicate), if you want to read more about it.

@jennybc jennybc requested a review from hadley March 13, 2017 06:48
src/utils.h Outdated
// If date is *prior* to the non-existent leap day: add a day
// If date is on the non-existent leap day: make negative and, in due course, NA
// Otherwise: do nothing
inline double removeLeapDay(double xlDate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be slightly easier to follow if you combined these two functions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good point. Done 2d57d7a

@jennybc jennybc merged commit c9a54ae into tidyverse:master Mar 13, 2017
@jennybc jennybc deleted the bugfix-leap-year branch March 14, 2017 17:51
@wjcgh
Copy link

wjcgh commented Apr 20, 2017

Thank you jennybc for making the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Difference of one day.
3 participants