Skip to content

Conversation

@marc-mabe
Copy link
Contributor

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • 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 marc-mabe force-pushed the fix-Date-roundMicroseconds branch from 58ade22 to cd8e837 Compare August 26, 2024 12:32
@oleibman
Copy link
Collaborator

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 oleibman added this pull request to the merge queue Sep 5, 2024
Merged via the queue into PHPOffice:master with commit fd96566 Sep 5, 2024
@oleibman
Copy link
Collaborator

oleibman commented Sep 5, 2024

Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants