-
Couldn't load subscription status.
- Fork 32
feat(parsing): support parsing leap seconds by normalizing 60 to 59 seconds #289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@gtkacz thanks for taking the time to pick this up. Looks good so far. Let me know if you have any questions/suggestions to improve the contributor experience. The contributor instructions are a bit light on detail at the moment. |
I do think there's a couple small things that the repo could use:
Nothing too major. You can definitely do everything you need by looking around for a couple minutes but it'd be cool to have it all in the contributing docs. I can do some of these pretty easily if you want, just let me know which ones and if I should associate them to issues and etc. Keep up the great work! :) |
|
@ariebovenberg testing is behaving weird. The pure Python implementations fails on Python > 3.10. Do you have any idea what could be causing this? |
The prime suspect would be that the two The reason there's 2 functions, is to ensure Python 3.11 takes advantage of the fast edit: typo |
|
Oh duh yeah, total brainfart on my part. Sorry for that, been having a busy week. Will fix it later today |
|
So @ariebovenberg I tried running the tests on Windows/Python 3.13.0 and WSL Ubuntu/Python 3.13.5 and both are being successful. I double checked both of my implementations of |
|
@gtkacz no problem, I'll have a look what's going on |
|
Thank you! Let me know if you find anything and I'll fix it :) |
|
It does seem to be a problem with given a basic time like # _pywhenever.py:5890
if s.endswith(":60"):
s = s[:-2] + "59"Leap seconds are only triggered in extended format, because it's checked for a colon |
|
You're right. I forgot that the second implementation has an explicit check for len == 6 that short circuits the colon implementation. Still, it doesn't make sense to me that the tests were passing on my machine lol |
|
I'm AFK at the moment, hope to review this weekend |
|
No rush! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Only some minor changes needed.
|
|
||
| def test_leap_seconds_parsing(self): | ||
| # Leap second (60) should be parsed and normalized to 59 | ||
| assert ZonedDateTime.parse_iso( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move these ISO-parsing test cases to the parametrized TestParseIso.test_valid test case. You can add comments above them that these are leap second test cases.
|
|
||
| def test_leap_seconds_comprehensive(self): | ||
| # Test with UTC - leap second normalization works across timezones | ||
| dt = ZonedDateTime.parse_iso("2020-08-15T12:34:60+00:00[UTC]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These parse_iso test cases can also be moved to TestParseIso.test_valid
|
@ariebovenberg been having a really busy week at work, sorry. I'll get around to the PR by Friday |
|
@gtkacz no problem. Be sure to rebase once you get back, there's been an 0.9.3 release in the meantime |
Description
Many standards and libraries allow for times like 23:59:60 to represent leap seconds.
whenevernow does so by allowing 60 seconds when ingesting or parsing data, but constraining it to 59 in the actual class. Closes #90.Checklist
Release checklist (maintainers only)
pyproject.tomlsrc/whenever/__init__.py