Skip to content

Replace pytz with zoneinfo#329

Merged
pcraciunoiu merged 3 commits intodjango-ses:mainfrom
go-product:fix/313-replace-pytz-with-zoneinfo
Aug 22, 2024
Merged

Replace pytz with zoneinfo#329
pcraciunoiu merged 3 commits intodjango-ses:mainfrom
go-product:fix/313-replace-pytz-with-zoneinfo

Conversation

@msopacua
Copy link
Copy Markdown
Contributor

@msopacua msopacua commented Aug 11, 2024

  • For python 3.8, add backports.zoneinfo and import accordingly. Python 3.8 is still supported for Django 4.2.
  • Make the localize argument to stats_to_list a boolean, since we no longer use a pytz-centric API.
  • pytz uses normalize() to deal with DST folding when changing from or to a DST timezone, however, zoneinfo handles this differently and doesn't require normalization.
  • While in here, add Django 5.1 to the test matrix

closes #313

@msopacua
Replace pytz with zoneinfo
5672dda 
- For python 3.8, add backports.zoneinfo and import accordingly. Python
  3.8 is still supported for Django 4.2.
- Make the localize argument to `stats_to_list` a boolean, since we no
  longer use a pytz-centric API.
- pytz uses normalize() to deal with DST folding when changing from or
  to a DST timezone, however, zoneinfo handles this differently and
  doesn't require normalization.
- While in here, add Django 5.1 to the test matrix
pcraciunoiu
pcraciunoiu approved these changes Aug 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@pcraciunoiu pcraciunoiu left a comment

Choose a reason for hiding this comment

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

Looks good to me!

  1. Before I merge and release, could you confirm that you tested manually as well?

  2. I didn't yet check, is there a unit test covering any of this timezone logic? If not, adding one would be great.

Thanks for contributing.

@msopacua
Copy link
Copy Markdown
Contributor Author

msopacua commented Aug 13, 2024

I ran the tests, thinking there would be a test for it, but if you're usure, I'll run coverage and add a test if needed.

@msopacua
Copy link
Copy Markdown
Contributor Author

msopacua commented Aug 13, 2024 

Name                     Stmts   Miss  Cover   Missing
------------------------------------------------------
django_ses/views.py        210     77    63%   12-13, 35-37, 101-144

So lines 41-62 are covered.
I haven't tested it myself yet, cause I'm paying this forward. I'm integrating into our stack this week, and saw the low hanging fruit. :)

@pcraciunoiu
Copy link
Copy Markdown
Contributor

pcraciunoiu commented Aug 13, 2024 

Name                     Stmts   Miss  Cover   Missing
------------------------------------------------------
django_ses/views.py        210     77    63%   12-13, 35-37, 101-144

So lines 41-62 are covered.
I haven't tested it myself yet, cause I'm paying this forward. I'm integrating into our stack this week, and saw the low hanging fruit. :)

Would you mind testing it from your branch before I make a release? That would give some assurance and avoid having to release a hotfix if you do find something.

@msopacua
test(313): Add a test that verifies tzinfo
2e91405 
Add a test that verifies that timestamps are localized to
settings.TIME_ZONE and hours adjusted accordingly.
@msopacua
Copy link
Copy Markdown
Contributor Author

msopacua commented Aug 13, 2024

So coverage was a bit misleading, there's not an actual test that looks at the timezone.
So I duplicated the test that calls with localize=False and then changed it to be called with localize=True and adjusted the hours.

One thing I noted was that override_settings doesn't work, likely because of the way the settings are initialized in runtests.py.

The default is America/Chicago, so I worked with that. Under the debugger, verified that it is the value of settings.TIME_ZONE when test is executed.

@pcraciunoiu
Copy link
Copy Markdown
Contributor

pcraciunoiu commented Aug 13, 2024

@msopacua great stuff. I wonder how hard it would be to fix the override settings for tests, it's unfortunate that doesn't work. May be the repo needs to be updated to import the django settings. But that seems out of scope for this PR (and I would prefer it to be separate anyways).

Looking at this more closely, the changes are purely for the stats code, so I'd be OK to make a release. I'd still like to wait until you run a manual test on a project to do so, that way you can test the code close to release time rather than it being released for a while.

That said, the code changes look good.

@msopacua
Copy link
Copy Markdown
Contributor Author

msopacua commented Aug 22, 2024

Screenshot 2024-08-22 at 16 39 44

Sorry for the delay. Tests are looking good and times are correct.

@pcraciunoiu pcraciunoiu merged commit 417b383 into django-ses:main Aug 22, 2024
@pcraciunoiu
Copy link
Copy Markdown
Contributor

pcraciunoiu commented Aug 22, 2024

Thanks!

Released as v4.1.1

https://pypi.org/project/django-ses/4.1.1/

@pcraciunoiu pcraciunoiu mentioned this pull request Nov 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcraciunoiu pcraciunoiu pcraciunoiu 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.

Replace pytz with zoneinfo

2 participants

@msopacua @pcraciunoiu