Skip to content

Conversation

kiblik
Copy link
Contributor

@kiblik kiblik commented Aug 12, 2025

Add and fix:

Move the fetching of timezone - to load the latest value

@github-actions github-actions bot added the lint label Aug 12, 2025
@kiblik kiblik force-pushed the ruff/DTZ002+011 branch 3 times, most recently from 1c41163 to 98e0b87 Compare August 12, 2025 12:11
@kiblik kiblik marked this pull request as ready for review August 12, 2025 12:12
Copy link
Contributor

@Maffooch Maffooch left a comment

Choose a reason for hiding this comment

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

I am curious how this behaves with the DD_TIME_ZONE env var

@kiblik
Copy link
Contributor Author

kiblik commented Aug 15, 2025

I am curious how this behaves with the DD_TIME_ZONE env var

Thanks for pointing it out. I forgot about this env var. In my opinion, it is quite confusing to have multiple places for setting the time zone.

During deeper analysis, I found some leftovers (addressed in #12995) that might simplify this PR.

I was also trying to find the reason for the existence of time_zone in system_setting.
Here it is #313 (wow, 3-digit PR, was some quite some time ago 😄).

In my opinion, the idea was nice and I support it in some way (to have more settings in system_setting so you would be able to change it without restarting of application). Unfortunately, with time_zone, it is a little bit complicated. I suppose that the native Django approach (using TIME_ZONE, loaded from env var DD_TIME_ZONE) is used in multiple places of application. The same setting of time zone setting should be used across the whole application. It should not be selectively loaded in different parts differently. And it is much harder for maintenance to refer to system_setting everywhere (it is easy to forget about it).

I'm willing to drop all get_system_setting("time_zone") and use only the Django native approach. If you agree.

@kiblik kiblik requested a review from Maffooch August 15, 2025 12:57
@Maffooch
Copy link
Contributor

I'm willing to drop all get_system_setting("time_zone") and use only the Django native approach. If you agree.

I do agree with this approach. It would be really nice to have the system setting working everywhere, but that would require a bit of a migration that could have some consequences if it didn't go perfectly. I'm definitely open to doing this in the future, but am not sure now is best time

@kiblik kiblik marked this pull request as draft August 15, 2025 15:43
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants