add microseconds support for Date/Time objects #2404
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Aug 17, 2023
This is a replacement for PR PHPOffice#2404 which has been open for almost 2 years, and which I will close now. As submitted, it broke many unit tests, and no attempt was made to fix those and add others. However, while reviewing it, I found that, among all the tests which it accidentally broke, there were tests which it "broke" (in ExplicitDateTest) which were actually wrong in the first place. That seemed a good enough reason to investigate further. The original PR suggested the change was needed because "there is now support for microseconds when reading Datetime cells". I'm not sure that's true. Time of day is stored as a fraction of a day, and nothing prevents microseconds from being part of that fraction. It is true that Excel does not allow you to format a date/time cell to display more than 3 decimal positions for the seconds value, even if the value turns out to be accurate to the microsecond, and that has not changed. It is also true that Php supports microsecond accuracy in its DateTime objects, and it behooves PhpSpreadsheet to accommodate that. PhpSpreadsheet, like Excel, nominally supported the use of one, two, or three decimals when displaying seconds. However, it did not do it correctly, and there had been no tests of this using a value where the decimals were anything other than 0. One existing test, in NumberFormatDates, was wrong. It is fixed and new tests added.
11 tasks
|
Supersed by PR #3677. Closing. |
oleibman
added a commit
that referenced
this pull request
Aug 23, 2023
This is a replacement for PR #2404 which has been open for almost 2 years, and which I will close now. As submitted, it broke many unit tests, and no attempt was made to fix those and add others. However, while reviewing it, I found that, among all the tests which it accidentally broke, there were tests which it "broke" (in ExplicitDateTest) which were actually wrong in the first place. That seemed a good enough reason to investigate further. The original PR suggested the change was needed because "there is now support for microseconds when reading Datetime cells". I'm not sure that's true. Time of day is stored as a fraction of a day, and nothing prevents microseconds from being part of that fraction. It is true that Excel does not allow you to format a date/time cell to display more than 3 decimal positions for the seconds value, even if the value turns out to be accurate to the microsecond, and that has not changed. It is also true that Php supports microsecond accuracy in its DateTime objects, and it behooves PhpSpreadsheet to accommodate that. PhpSpreadsheet, like Excel, nominally supported the use of one, two, or three decimals when displaying seconds. However, it did not do it correctly, and there had been no tests of this using a value where the decimals were anything other than 0. One existing test, in NumberFormatDates, was wrong. It is fixed and new tests added.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is:
Checklist:
Why this change is needed?
Because there is now support for microseconds when reading Datetime cells