BUG: fix parsing of ODF time values with comments

[mzuzek]

• Similar to: BUG: Comment in ODS-file gets included in string cells #55200 variant for time-value cells
• Tests: added comments to cells in test_1900.ods and test_1904.ods fixtures, so that io/excel/test_readers.py:test_reader_seconds() would be failing without this fix. Also fixed missing microseconds there (see p.1 in BUG (test): bad file for testing ODFReader #55045).
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/v2.2.0.rst file.

ODF office:time-value format is specified by p.3.2.6 of XML Schema (Part 2) as referenced in p.19.389 of OpenDocument Schema Part 3

Related to: #55045 (test files updates)

[dimastbk]

ODS supports timedelta ([hh]:mm:ss). times_1904.ods is broken, see #55045. Maybe fix the file and do something like?

if h > 23:
    return pd.Timedelta(...)
else:
    return cast(Scalar, datetime.time(...))

[mzuzek]

Right, timedelta seem to make more sense than timestamp - at least for ODF time-value, because durations can be larger than 24h and they can be negative, see duration.ods:

It's my first PR here, so I thought I'd better keep it tight and clean - hoping we could leave timedelta for a follow-up discussion and new PR. For sure unit tests would need adjustments (they specifically require datetime.time timestamps) and I don't know how other spreadsheet formats correspond.

[rhshadrach]

Am I correct in saying that the current behavior of pandas will read the top value in the screenshot above as having 50 hours, where as this change will now be 50 - 48 = 2 hours?

[mzuzek]

@rhshadrach Which commit/version of pandas yields 50 hours for you ?
I thought you'd get an error in _odfreader.py:217 similar to:

>>> pd.Timestamp('50:15:00') 
Traceback (most recent call last):
  File "parsing.pyx", line 681, in pandas._libs.tslibs.parsing.dateutil_parse
ValueError: hour must be in 0..23

[rhshadrach]

Which commit/version of pandas yields 50 hours for you ?

I haven't run anything.

I thought you'd get an error in _odfreader.py:217 similar to:

I think you're saying that both main and this PR will raise on duration.ods, is that right?

[mzuzek]

Nope, I only said that the current implementation does not support time value equal or larger than 24 hours in ODF files.

Could you please run the files you'd like to check and share any results that raise your concerns ?

[rhshadrach]

I only said that the current implementation

Does current implementation mean main or this PR?

[mzuzek]

On a493bb7, I've refactored the helper function into _get_cell_time_value() - similar to existing _get_cell_string_value() for strings.
Pardon me not grasping the ODFReader class layout in the first place...

[mzuzek]

@rhshadrach @mroeschke @dimastbk

Would there be anything else I should do on this PR ?

[mroeschke]

Looks OK to me

[rhshadrach]

mzuzek force-pushed the fix-odf-time-comments branch from 7c22815 to ea23a2e
yesterday

Just so you're aware, by force pushing you make it so that reviewers can no longer use the "Show changes since your last review" feature. Not a big deal at all here because the diff is so small.

[rhshadrach]

I have some questions - see the above review comments.

[github-actions]

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

[mroeschke]

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

[mzuzek]

@rhshadrach @mroeschke @dimastbk

Thank you for reviewing this PR and for all your comments.

I'm sorry it didn't work out:
I understand that after all review issues and requests have been properly addressed - as confirmed here -
this PR was blocked by discussions which haven't introduced any new problems or change requests (last one here).
I'll assume it's just the way things work here.