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

Adds internal timezone_from_offset function #3648

Merged
merged 1 commit into from Dec 15, 2023

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Dec 14, 2023

Adds internal timezone_from_offset function

It allows to build conversions from chrono without direct access to the C API

@Tpt
Copy link
Contributor Author

Tpt commented Dec 14, 2023

This change is a first step for #3633. It allows to remove all C API calls from the chrono conversion.

I am not sure adding two functions is the best way to go. An other option might be to convert the three functions constructing datetime.timezone to associated functions of the PyTzInfo object.

@Tpt Tpt force-pushed the timezone-constructor branch 2 times, most recently from c5a6c97 to 8ec500b Compare December 14, 2023 16:37
@davidhewitt
Copy link
Member

Note that there is a long history of why these functions don't exist, see #1588 (comment)

I'm not totally against adding them, but also do understand why others did not want them.

@Tpt Tpt changed the title Adds timezone_from_offset and timezone_from_offset_and_name functions Adds internal timezone_from_offset function Dec 14, 2023
@Tpt Tpt force-pushed the timezone-constructor branch 3 times, most recently from 9dc3ea0 to 418e0cb Compare December 14, 2023 20:35
@Tpt
Copy link
Contributor Author

Tpt commented Dec 14, 2023

Note that there is a long history of why these functions don't exist, see #1588 (comment)

I'm not totally against adding them, but also do understand why others did not want them.

Thank you! I was not aware of this discussion. The concerns make sense. I have rewritten this MR to make timezone_from_offset private and I dropped timezone_from_offset_and_name.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Dec 14, 2023
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Overall this now looks like it just relocated the function, but I've no problem with that if it's a helpful first step for the follow up feature work 👍

newsfragments/3648.changed.md Outdated Show resolved Hide resolved
It allows to build conversions from chrono without direct access to the C API
@davidhewitt davidhewitt added this pull request to the merge queue Dec 15, 2023
Merged via the queue into PyO3:main with commit 867a273 Dec 15, 2023
37 checks passed
@Tpt Tpt deleted the timezone-constructor branch December 15, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants