Skip to content

Conversation

@gtkacz
Copy link
Contributor

@gtkacz gtkacz commented Sep 29, 2025

Description

Many standards and libraries allow for times like 23:59:60 to represent leap seconds. whenever now does so by allowing 60 seconds when ingesting or parsing data, but constraining it to 59 in the actual class. Closes #90.

Checklist

  • Build runs successfully
  • Type stubs updated
  • Docs updated
  • If docstrings were affected, check if they appear correctly in the docs as well as autocomplete

Release checklist (maintainers only)

  • Version updated in pyproject.toml
  • Version updated in src/whenever/__init__.py
  • Version updated in changelog
  • Branch merged
  • Tag created and pushed
  • Confirm publish job runs successfully
  • Github release created

@ariebovenberg
Copy link
Owner

ariebovenberg commented Sep 29, 2025

@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.

@gtkacz
Copy link
Contributor Author

gtkacz commented Sep 30, 2025

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:

  1. Use something like this to make the repo more accessible to Windows devs. I personally use WSL so I could run the Makefile no problem, but it couldn't hurt to make developing more accessible.
  2. Maybe something like pre-commit so that some of the quality checks can be run locally and not wait for GHA?
  3. The CONTRIBUTING.md document and/or PR templates definitely could use some more detailing IMO. For example, in the PR checklist, it's not immediately clear what exactly "Type stubs updated" is or how/when should I do it. Something like how to run/test the docs could be useful as well for someone not familiar with Sphinx.
  4. Issue templates for non-coding contributions. Should be easy enough to essentially copy from another repo.

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! :)

@gtkacz
Copy link
Contributor Author

gtkacz commented Sep 30, 2025

@ariebovenberg testing is behaving weird. The pure Python implementations fails on Python > 3.10. Do you have any idea what could be causing this?

@ariebovenberg
Copy link
Owner

ariebovenberg commented Sep 30, 2025

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 __time_from_iso_nofrac implementations are out-of-sync. There's one for >=Py3.11 and one for <Py3.11. I see you've found both, but probably there's an issue with one of them.

The reason there's 2 functions, is to ensure Python 3.11 takes advantage of the fast fromisoformat of the standard library. However, if the code becomes longer due to leap second handling then we can probably just merge it into one function for maintainability.

edit: typo

@gtkacz
Copy link
Contributor Author

gtkacz commented Oct 1, 2025

Oh duh yeah, total brainfart on my part. Sorry for that, been having a busy week. Will fix it later today

@gtkacz
Copy link
Contributor Author

gtkacz commented Oct 1, 2025

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 __time_from_iso_nofrac and they seem to be mirrored correctly. I'm using uv and rye for Python installation/management if that makes any difference

@ariebovenberg
Copy link
Owner

@gtkacz no problem, I'll have a look what's going on

@gtkacz
Copy link
Contributor Author

gtkacz commented Oct 1, 2025

Thank you! Let me know if you find anything and I'll fix it :)

@ariebovenberg
Copy link
Owner

ariebovenberg commented Oct 1, 2025

It does seem to be a problem with __time_from_iso_nofrac.

given a basic time like 051260 (i.e. 05:12:60), this line seems to be the problem:

# _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

@gtkacz
Copy link
Contributor Author

gtkacz commented Oct 1, 2025

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

@gtkacz gtkacz marked this pull request as ready for review October 2, 2025 13:50
@ariebovenberg
Copy link
Owner

I'm AFK at the moment, hope to review this weekend

@gtkacz
Copy link
Contributor Author

gtkacz commented Oct 2, 2025

No rush! :)

Copy link
Owner

@ariebovenberg ariebovenberg left a 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(
Copy link
Owner

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]")
Copy link
Owner

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

@gtkacz
Copy link
Contributor Author

gtkacz commented Oct 9, 2025

@ariebovenberg been having a really busy week at work, sorry. I'll get around to the PR by Friday

@ariebovenberg
Copy link
Owner

@gtkacz no problem. Be sure to rebase once you get back, there's been an 0.9.3 release in the meantime

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow parsing leap seconds

2 participants