PhpSpreadsheet icon indicating copy to clipboard operation
PhpSpreadsheet copied to clipboard

Fix Date::roundMicroseconds() not resetting microsecond part

Open marc-mabe opened this issue 1 year ago • 1 comments

This is:

  • [x] a bugfix
  • [ ] a new feature
  • [ ] refactoring
  • [x] additional unit tests

Checklist:

  • [x] Changes are covered by unit tests
    • [ ] Changes are covered by existing unit tests
    • [x] New unit tests have been added
  • [x] Code style is respected
  • [ ] Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • [ ] CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • [ ] Documentation is updated as necessary

Why this change is needed?

Date::roundMicroseconds() was increasing the seconds by 1 but wasn't resetting the microseconds. I'm using the underlying DateTime object created by Date::excelToDateTimeObject() to not loose precision but as excel uses a floating-point number to store this value I got bitten by parsing values like 2001-02-03 04:05:06 ending up in 2001-02-03 04:05:05.999996. As I have seen you are now rounding this value to seconds I noticed the value was still wrong as it still had the microsecond part left while the second got increased by 1.

PS 1: Date::roundMicroseconds() is a really weird name for a function to round to seconds PS 2: It's great that Date::excelToDateTimeObject() supports microseconds now but as the underlying value is a floating point number it would be much appreciated if it could be rounded to a value not loosing precession. How does LibreOffice (I don't have excel) handle it as 2001-02-03 04:05:05.000000 just works there?

marc-mabe avatar Aug 26 '24 12:08 marc-mabe

roundMicroseconds was intended merely as a helper for the date piece functions (YEAR, MONTH, DAY, HOUR, MINUTE, SECOND), and for excelToTimestamp, whose doc-block specifies that its use is discouraged and which drops the fractional seconds anyhow. Your change isn't needed for any of those, but also doesn't appear to harm them. Since the function is public and you have a use case for your change, I'm inclined to proceed with it.

Sorry if the name is confusing - it seemed like a good choice when it was introduced. Water under the bridge now.

I do not understand your comment about LibreOffice. Please elaborate.

oleibman avatar Aug 26 '24 15:08 oleibman

Thank you for your contribution.

oleibman avatar Sep 05 '24 16:09 oleibman