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.

Sign up for GitHub

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

Jump to bottom

use datetime.fold to distinguish ambiguous datetimes when converting #4791

Merged
merged 2 commits into from
Dec 16, 2024
Merged

use datetime.fold to distinguish ambiguous datetimes when converting #4791

merged 2 commits into from
Dec 16, 2024

Conversation

bschoenmaeckers
Copy link
Contributor

fixes #4722

@bschoenmaeckers
Copy link
Contributor Author

bschoenmaeckers commented Dec 12, 2024 •
edited
Loading

We should probably do this for the IntoPyObject case as well. Current implementation will always set fold to 0.

@bschoenmaeckers bschoenmaeckers force-pushed the chrono-ambiguous branch 6 times, most recently from 0c0c588 to 36be8a8 Compare December 13, 2024 10:25
Icxolu
Icxolu reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

@Icxolu Icxolu left a comment

Choose a reason for hiding this comment

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

We should probably do this for the IntoPyObject case as well. Current implementation will always set fold to 0.

Thanks! Agreed. Implementation looks good to me. Perhaps we can add a round-trip test converting (non)ambiguous DateTime<Tz> into Python, then extracting them back and make sure they are the same? What do you think?

@bschoenmaeckers
Copy link
Contributor Author

We should probably do this for the IntoPyObject case as well. Current implementation will always set fold to 0.

Thanks! Agreed. Implementation looks good to me. Perhaps we can add a round-trip test converting (non)ambiguous DateTime<Tz> into Python, then extracting them back and make sure they are the same? What do you think?

Current test verifies this behaviour as well

Icxolu
Icxolu approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@Icxolu Icxolu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Current test verifies this behaviour as well

Oops, I must have overlooked that part.

@Icxolu Icxolu added this pull request to the merge queue Dec 16, 2024
Merged via the queue into PyO3:main with commit b17de4f Dec 16, 2024
46 checks passed
@bschoenmaeckers
Copy link
Contributor Author

Great! Thanks for looking at my changes!

davidhewitt pushed a commit that referenced this pull request Jan 3, 2025
@bschoenmaeckers @davidhewitt
use datetime.fold to distinguish ambiguous datetimes when converting (
5011a98 
#4791)

* use `datetime.fold` to distinguish ambiguous datetimes when converting

* Set correct fold when converting to ambiguous `chrono::DateTime<Tz>`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Icxolu Icxolu Icxolu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DateTime<Tz>'s implementation for FromPyObject does not consider Pythons's datetime.fold attribute
2 participants
@bschoenmaeckers @Icxolu