Skip to content

Remove pytz dependency, require Python 3.9+, extend tests#43

Merged
justinmayer merged 7 commits intogetpelican:mainfrom
do3cc:zoneinfo
Jul 14, 2025
Merged

Remove pytz dependency, require Python 3.9+, extend tests#43
justinmayer merged 7 commits intogetpelican:mainfrom
do3cc:zoneinfo

Conversation

@do3cc
Copy link
Contributor

@do3cc do3cc commented Mar 10, 2025

This is the PR to implement #42
I've reviewed timezone/utils.py and noticed a lot of code isn´t used at all.
Then I added tests for windows and macos to make sure, nothing breaks.
Since nothing broke, I got worried and extended the tests to use timezones as provided by Python 3.9.
Then my windows tests failed and I added the necessary dependency for windows.
Now that I am writing this, I am wondering whether we should depend on tzinfo, for windows, as we don´t use the available timezones anywhere. However, I like that there are tests for that.

@justinmayer
Copy link
Member

Thanks for your work on this, Patrick.

@venthur / @hugovk: I am preparing for a trip and could use an extra pair of eyes. Would you mind taking a look and providing any feedback you may have?

@do3cc
Copy link
Contributor Author

do3cc commented Mar 11, 2025

After this I also downloaded pelican and made a test run with my modified branch. No failures. However I saw a few skipped tests due to missing locales.

Copy link
Contributor

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Also:

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 5b811c0..d2d7027 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -1,7 +1,7 @@
 ---
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v4.6.0
+    rev: v5.0.0
     hooks:
       - id: check-added-large-files
       - id: check-ast
@@ -15,7 +15,7 @@ repos:
       - id: trailing-whitespace
 
   - repo: https://github.com/asottile/pyupgrade
-    rev: v3.15.2
+    rev: v3.19.1
     hooks:
       - id: pyupgrade
-        args: [--py38-plus]
+        args: [--py39-plus]

@do3cc
Copy link
Contributor Author

do3cc commented Mar 12, 2025

Everything addressed. Thanks for the review

@venthur
Copy link
Collaborator

venthur commented Mar 13, 2025

Thanks for your contribution and addressing all requests @do3cc! @justinmayer the PR looks good to go.

@justinmayer
Copy link
Member

Hi Patrick. I am so sorry, but I merged #44 and #45 before realizing that your contribution had not been merged yet. If it is not too much to ask, would you be willing to rebase on current main and adjust the commits on this branch to incorporate the parts of your changes that are still relevant?

@do3cc
Copy link
Contributor Author

do3cc commented Jul 14, 2025

No worries, I just did so.

@justinmayer justinmayer changed the title Remove pytz dependency, update minimum python version to 3.9, extend tests Remove pytz dependency, require Python 3.9+, extend tests Jul 14, 2025
@do3cc
Copy link
Contributor Author

do3cc commented Jul 14, 2025

I have also changed the github workflow again. I got some feedback from earlier reviews not do the optimizations that I just reverted and I somehow managed to loose them during the refactoring.

Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

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

Many thanks to @do3cc for the enhancements (and patience) as well as to @venthur and @hugovk for reviewing! 🥇

@justinmayer justinmayer merged commit 9cb163a into getpelican:main Jul 14, 2025
19 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.

4 participants