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

Django5 #179

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Django5 #179

merged 3 commits into from
Dec 6, 2023

Conversation

norkans7
Copy link
Contributor

@norkans7 norkans7 commented Dec 5, 2023

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (115f821) 81.75% compared to head (fff6b83) 81.75%.

Files Patch % Lines
smartmin/tests.py 0.00% 4 Missing ⚠️
smartmin/templatetags/smartmin.py 25.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #179   +/-   ##
=======================================
  Coverage   81.75%   81.75%           
=======================================
  Files          33       33           
  Lines        2220     2220           
=======================================
  Hits         1815     1815           
  Misses        405      405           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -222,7 +222,7 @@ def import_xls(cls, filename, user, import_params, log=None, import_results=None
naive_timezone = (
zoneinfo.ZoneInfo(import_params["timezone"])
if import_params and "timezone" in import_params
else timezone.utc
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused.. isn't timezone.utc fine to use? I think we just need to set USE_DEPRECATED_PYTZ = False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an alias to datetime.timezone.utc and that is removed in django 5.0

Copy link
Contributor Author

@norkans7 norkans7 Dec 5, 2023

Choose a reason for hiding this comment

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

USE_DEPRECATED_PYTZ is removed as well

Copy link
Member

Choose a reason for hiding this comment

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

Arrghh all this time I've been replacing pytz.UTC with django.utils.timezone.utc

I see the old warning message now is:

The django.utils.timezone.utc alias is deprecated. 
Please update your code to use datetime.timezone.utc instead.

So maybe we should be using datetime.timezone.utc ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had tried that in a54199d but I did not like that since I had to alias datetime.timezone to not collide with django.utils.timezone still used for things like timezone.now()

Copy link
Member

Choose a reason for hiding this comment

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

I think I'm leaning toward always accessing it like datetime.timezone.utc and then continuing to use timezone.now() like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I guess on our side we can just decide to consistently use the alias approach for now
from datetime import timezone as datetime_timezone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree we need to keep timezone.now() but datetime.timezone.utc has issues since we mostly import
from datetime import datetime, timedelta

I think we can use the alias for timezone
from datetime import timezone as datetime_timezone

That will easily for for the existing imports

Copy link
Member

Choose a reason for hiding this comment

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

Can we pick something shorter than datetime_timezone? tzone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll use tzone

@rowanseymour rowanseymour merged commit f311c1c into main Dec 6, 2023
26 checks passed
@rowanseymour rowanseymour deleted the django5 branch December 6, 2023 14:52
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