Skip to content
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

Add note / warning about non-existent datetimes #180

Closed
wants to merge 1 commit into from

Conversation

benthorner
Copy link
Contributor

In response to [^1].

This is a known issue with datetime [^2].

[^1]: #178 (comment)
[^2]: https://peps.python.org/pep-0495/#the-fold-attribute
@benthorner benthorner force-pushed the datetime-dst-warning branch from 8f83a6d to fc88871 Compare December 9, 2024 14:43
@benthorner benthorner requested a review from jarshwah December 9, 2024 14:43
@benthorner benthorner marked this pull request as ready for review December 9, 2024 14:43
# datetime(2020, 2, 29, hours=1, tzinfo=london_tz)
# => timedelta(hours=1) <== WRONG (should be 0)
#
# with pytest.raises(ValueError): # empty range
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ranges shouldn't be referenced in localtime - they are independent.

@@ -35,6 +70,8 @@ def as_localtime(
Convert a tz aware datetime to localtime.

Wrapper for the `django.utils.timezone` function, taking the same arguments.

Warning: See Note [Non-existent datetimes]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is helpful here. The warnings will only be visible to authors of this package, not callers (who will see the warning in the doc-string.

But also, I'm not sure it has any relation to this function. Once a datetime is localised, it will be fine.

The information written is good, I just don't think it has relevance being linked in this docstring.

@benthorner
Copy link
Contributor Author

Thanks for the feedback @jarshwah 💯.

I agree this isn't the best place for this documentation. Arguably nowhere is as it's just documentation about Python itself - localtime isn't really doing anything differently or wrong, just making it more convenient.

Here's another attempt, but proper documentation this time: #182.

I've tried to frame it more as documentation about localtime, through which we then discuss the pitfalls. Although it's not our code that's the problem, I think we need "reachable" docs about it somewhere.

@benthorner
Copy link
Contributor Author

Closing in favour of #182.

@benthorner benthorner closed this Dec 18, 2024
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.

2 participants