Skip to content

Conversation

@torotil
Copy link
Contributor

@torotil torotil commented Sep 6, 2024

fixes #754

@kurtmckee kurtmckee merged commit 19a0a10 into kvesteri:master Sep 6, 2024
@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