Skip to content

Make datetime rust wrappers compatible with abi3 feature#4970

Merged
davidhewitt merged 5 commits intoPyO3:mainfrom
bschoenmaeckers:abi3-datetime
Apr 4, 2025
Merged

Make datetime rust wrappers compatible with abi3 feature#4970
davidhewitt merged 5 commits intoPyO3:mainfrom
bschoenmaeckers:abi3-datetime

Conversation

@bschoenmaeckers
Copy link
Member

@bschoenmaeckers bschoenmaeckers commented Mar 11, 2025

This PR makes PyDate, PyTime, PyDateTime, PyDelta, and PyTzInfo available in abi3 builds. However, PyDateAccess, PyTimeAccess, and PyDeltaAccess have not been enabled, as they can fail under abi3, and their return values are not PyResult, making error handling problematic.

For now, fields in these wrappers should still be accessed using getattr, though we may consider changing this in the future.

@bschoenmaeckers bschoenmaeckers force-pushed the abi3-datetime branch 8 times, most recently from 27419dc to a5615a2 Compare March 14, 2025 10:37
@davidhewitt
Copy link
Member

Thanks for this one! While I can't find the discussion right now, I'm sure in the past something like this was proposed and at the time we chose not to add it. Quite often we've preferred not to add types in Rust for types that don't have a C API.

That said, these datetime bindings are a bit of a special case given they do have a C API on the unlimited build. So it feels like maybe exposing them on all builds is the right choice, and we should view this more like a performance optimization when not running abi3. It would certainly simplify code in PyO3 and I'm sure downstream can benefit too.

I'd be interested to hear if fellow maintainers have strong feelings we should not do this?

@davidhewitt
Copy link
Member

(Otherwise I suggest we move ahead 👍)

@Icxolu
Copy link
Member

Icxolu commented Mar 18, 2025

This seems fine to me, so 👍 from me as well. Having a unified interface seems generally easier to maintain an easier to use.

@@ -0,0 +1 @@
- Added `utc`, `timezone` and `from_offset` constructors to 'PyTzInfo'
Copy link
Member

Choose a reason for hiding this comment

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

There is a long history of why those functions didn't exist, see #1588 (comment)

Maybe now we have easy ways to convert chrono timezones to TzInfo, these APIs are sort of available in a roundabout way anyway.

That said, I would prefer split these API changes out from this PR and focus on abi3 support here, if that's ok please?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the TzInfo constructors change from this PR and will follow this up in a future PR.

@bschoenmaeckers bschoenmaeckers force-pushed the abi3-datetime branch 3 times, most recently from 78e5b8a to 950e455 Compare March 21, 2025 10:51
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.

Thanks, this is a welcome simplification and usability improvement!

@davidhewitt davidhewitt added this pull request to the merge queue Apr 4, 2025
@davidhewitt davidhewitt removed this pull request from the merge queue due to a manual request Apr 4, 2025
@davidhewitt
Copy link
Member

(Need #5041 before can merge...)

@davidhewitt davidhewitt added this pull request to the merge queue Apr 4, 2025
Merged via the queue into PyO3:main with commit ddcfc69 Apr 4, 2025
51 of 52 checks passed
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.

3 participants