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

fix(pendulum): Make PendulumDateTime work with postgres #755

Merged
merged 2 commits into from
Sep 6, 2024

Conversation

torotil
Copy link
Contributor

@torotil torotil commented Sep 6, 2024

fixes #754

@kurtmckee kurtmckee merged commit 19a0a10 into kvesteri:master Sep 6, 2024
11 checks passed
@kurtmckee
Copy link
Collaborator

Thanks for this fix, and for including a test case to demonstrate it!

@torotil torotil deleted the fix-754 branch September 6, 2024 12:28
@torotil
Copy link
Contributor Author

torotil commented Sep 6, 2024

… and thanks to you for merging this so quickly!

@torotil
Copy link
Contributor Author

torotil commented Sep 30, 2024

@kurtmckee I have now taken a more in-depth look at the code in enriched_datetime. To me it seems a bit overly complicated. I think the indirection using the EnrichedDate / EnrichedDateTime objects is not actually worth it. There is almost no code shared between those classes. I have made a draft of removing that (and updating the the docs). Are you interested in getting a PR in that direction? (The draft can be found here: master...torotil:sqlalchemy-utils:refactor-enriched_datetime)

I also discovered that the arrow implementation already handles both timezone-aware and naive objects while the pendulum implementation seems to only handle naive ones (especially after the changes in this PR). I suppose this would need more test cases on the pendulum side.

@kurtmckee
Copy link
Collaborator

PRs are welcome, but I hope that there will be others with more knowledge of the codebase that @kvesteri is able to bring on board for maintenance.

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.

pendulum datetime fails to preserve the timezone
2 participants