-
Notifications
You must be signed in to change notification settings - Fork 65
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
Be Excel-compatible when handling pre 1900-03-01 dates #146
Be Excel-compatible when handling pre 1900-03-01 dates #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a changelog entry makes total sense.
As for the PR itself - only a minor comment about haddocks, otherwise LGTM.
src/Codec/Xlsx/Types/Common.hs
Outdated
fractionOfOneDay = realToFrac diffTime / (24 * 60 * 60) | ||
-- For historical reasons, Excel thinks 1900 is a leap year (https://docs.microsoft.com/en-gb/office/troubleshoot/excel/wrongly-assumes-1900-is-leap-year). | ||
-- So dates strictly before March 1st 1900 need to be corrected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about moving this into haddock to make it visible to people using the function? For sure dateFromNumber
gives some insight but that's a function for the opposite conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I've changed the haddocks to contain this link and some more info as well, and noticed that this new behaviour makes dateFromNumber
and dateToNumber
no longer inverses of each other under certain circumstances, which I also documented.
I added an entry in the changelog but I don't know how you want to handle versioning, so feel free to change, suggest a change or just ask me to change something there. |
Sorry, was a bit busy - I plan to take a look into it this evening |
I think your change makes sense, I want to release a new version with #144 merged so could change the version but that's not done yet. |
This hopefully fixes #145.
Although this should bring us closer to Excel, it does break compatibility, so it's probably worth mentioning in the changelog?
Thanks for your work and please let me know any changes you might want!