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

Replace timezonefinder with tzfpy #176

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mvexel
Copy link
Contributor

@mvexel mvexel commented Jul 11, 2024

There's some advantages of tzfpy to timezonefinder. tzfpy uses tzf under the hood, so it introduces no python dependencies such as numpy. It's faster, in particular close to TZ boundaries. It has a very similar API for the simple purposes this project requires. Also timezonefinder is looking for a new maintainer, so this is hedging against that project becoming unmaintained.

I applied black formatting to the files I touched, but happy to comply with whatever code hygiene standards you have.

Thanks for considering.

mvexel added 8 commits July 11, 2024 11:18
1. add GDAL to dev dependencies
2. `poetry update`, mainly to pull in a newer `requests` to replace 2.32.0 which has a security vulnerability
the timezonefinder package [does not seem to be maintained anymore](https://github.com/jannikmi/timezonefinder/tree/master?tab=readme-ov-file#notice-looking-for-maintainers-reach-out-if-you-want-to-contribute). Moreover, it introduces numpy as a transient dependency, which seems heavy-handed.
This reverts commit b140050.
Does not belong in this PR :)
@thomersch
Copy link
Owner

I applied black formatting to the files I touched, but happy to comply with whatever code hygiene standards you have.

Perfect! CI actually tests for black formatting and it complains about two files: https://github.com/thomersch/openstreetmap-calendar/actions/runs/9899099646/job/27347358399

Not sure why that's the case, maybe a different config?

timezonefinder is looking for a new maintainer, so this is hedging against that project becoming unmaintained.

Phew, yeah… the dependencies are slowly getting stale.

I am a little bit conflicted about tzfpy: It would be nice to move to a package that gets more attention, but on the other hand it introduces a dependency on Rust and it's a fairly niche repo – or is it just my own paranoia?

Have you by any chance verified that it works with the manual time zone selection in the event creation/edit form? (It probably does, but I remember having inconsistencies with time zone names coming from the operating system db)

@ringsaturn
Copy link

Thanks for interested in tzfpy. It's true that it's not a polular project compared with timezonefinder since it was created to solve timezonefinder's performance issues instead of basic feature issues. And I believe only a few projects will face the performance issues.

By the way, Rust is not required unless wheel files are missing for the python verison and platform.

@mvexel
Copy link
Contributor Author

mvexel commented Jul 29, 2024

Thanks @ringsaturn for participating here.

Have you by any chance verified that it works with the manual time zone selection in the event creation/edit form? (It probably does, but I remember having inconsistencies with time zone names coming from the operating system db)

I have not. Whenever I get a chance..

Strange, those black errors. Isn't black supposed to be "no-config"? :)

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