xlnt-community / xlnt

:bar_chart: Cross-platform user-friendly xlsx library for C++11+
https://xlnt-community.github.io/xlnt/
Other
14 stars 3 forks source link

Time improvements #27

Closed doomlaur closed 1 month ago

doomlaur commented 2 months ago

As promised, this pull request does the following:

  1. Moves the wrappers for the C time functions to a common file (they no longer exist twice).
  2. Uses localtime_r on Linux and other Unix systems that support it to improve thread-safety.
  3. Fixes the unit test for weekdays under MSVC.

Note: the improved wrappers for the C time functions are based on the previous ones, but contain the improvements stated above. However, this also means that the new wrappers - just like the old ones - don't have any handling for failed calls to localtime or gmtime. For example, under MSVC, the C Standard Library of MSVC has the limitation for localtime, gmtime and mktime that it only supports dates between January 1st 1970 00:00:00 UTC and December 31st 3000 23:59:59 UTC. However, Excel supports dates beginning with January 1st, 1900, while XLNT under MSVC would not handle it correctly, without any information to the caller of our safe functions that something failed. We could change that by returning an xlnt::optional in case we detect that something has failed. Currently, the following will happen:

I definitely think that this is not great and should be improved - but I created this pull request to first ask what you @m7913d think about my proposed solution :slightly_smiling_face:

coveralls commented 2 months ago

Pull Request Test Coverage Report for Build db3b7966-1e9d-4fbf-ab64-e115ee9942d8

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
source/utils/datetime.cpp 51 52 98.08%
source/utils/date.cpp 29 31 93.55%
source/utils/time.cpp 0 4 0.0%
<!-- Total: 122 129 94.57% -->
Totals Coverage Status
Change from base Build dc7e29c9-72c4-42b0-a61d-2e9c99f48c75: 0.06%
Covered Lines: 10720
Relevant Lines: 12883

💛 - Coveralls
m7913d commented 2 months ago

As a minor remark, coverage is decreased due to the is_null conditions not being unit-tested (ensure exception is thrown, resulting string is empty). See coverage report.

doomlaur commented 1 month ago

Good point, thank you! I added unit tests for covering all cases that might fail due to an invalid xlnt::date or xlnt::datetime.

doomlaur commented 1 month ago

And, once again, CI has caught a case where all unit tests passed on MSVC, but failed on Linux. Nice! :smile: