Add tz argument to some localtime functions #88
Conversation
| ) -> datetime_.datetime: | ||
| """ | ||
| Return a datetime in the local timezone. | ||
| Return a datetime in the local timezone, or in the desired timezone if tz is provided. |
There was a problem hiding this comment.
❓ This module is just helpers around putting things in a specific (local) timezone -- if we want to use arbitrary timezones, why don't we just use django.utils.timezone directly?
In particular, make_aware_assuming_local(dt, not_local_tz) seems very strange to me.
There was a problem hiding this comment.
It is as strange as the midnight function (for example). localtime.midnight() allows to pass a tz to indicate a localtime in a timezone that is not the default one. At the end, most of the functions in the localtime module provide an optional tz so we can use the same library for non-default timezones.
This is used by the industrytime module and the with_industry_timezone wrapper. If you look at the definition of most of those functions, they are just wrapping the equivalent localtime function with the decorator, avoiding a lot of duplication (and testing). Some exceptions where we are duplicating the same code but with a different timezone are make_aware and datetime. This can be avoided if we add the tz to these functions in this module.
There was a problem hiding this comment.
I think you've identified a problem, but this isn't the right fix for it.
I think we're using this module to do two things: provide date times in the "local" timezone and a set of datetime utilities. We should probably make those two jobs two different modules: something that contains a tz-agnostic set of helpers (midnight, next-midnight, days_in_past, etc) and then a module that wraps those and injects a specific timezone like industrytime does in kraken-core.
I think the clearest indication of that is that one of these functions will have 'as localtime' in the name and will allow you to set any timezone and does nothing that just using the Django timezone package doesn't do.
If there's absolutely no way we can make that change and this needs to go in urgently, then I could accept the compromise of following the pattern of the rest of the module, as long as someone commits to fixing it later. Apart from the function with localtime in its name -- that one seems too wrong to me.
There was a problem hiding this comment.
I do agree that this module is providing both, a series of functions that depends on timezones and a series of helpers for handling datetime operations.
I think though, that "the local timezone" is a deprecated concept that worked very well in the UK (where there is only one timezone), but does not work in Australia or any other territory or market with several timezones. We don't talk about the "localtime" anymore, but about the "localtime in Sydney", the "localtime in Canary Islands" or the "localtime for the Australian Electricity Industry".
I do agree that probably the entire concept of this "localtime" module needs to be re-thought because of that, but the intention of this PR was only to make a first step (making the functions that should be "timezone relative" to accept a timezone) and this way avoiding multiple reimplementations of the same functions which could make things harder to refactor in the future.
This is not urgent, it was part of my last Spa Day because I don't usually have enough time to work on this.
Feel free to approve this PR or close it if you think you can provide a better alternative to this problem 👍
There was a problem hiding this comment.
I think though, that "the local timezone" is a deprecated concept
I'm not aware of this being the case. There are times when using the Kraken instance's "home" timezone is not always appropriate, but there are a lot of systems (notably all of the financials systems) which rely on there being a single timezone to use as a reference. Can you point to the discussion.decision where this concept was deprecated?
There was a problem hiding this comment.
Let me explain better: I'm not saying that accessing the time configured behind the settings.TIME_ZONE is not useful, this is clearly very necessary to have a point of reference for ES to know "when an email was sent", when a customer called", etc etc. What I'm saying is that tying the localtime module to only that timezone and calling it "the local timezone" is a deprecated practice since in Australia not only supply periods, but also agreements (and bills) and apeps are aligned to the industry timezone.
One place with discussion around how to create logic that can be used to supersede the "one localtime" concept was the channel #temp-localtime-as-industrytime where there was some discussion on whether we should still have only one localtime (but set its timezone to the industry timezone).
Eventually, we realised the problem is not about changing the timezone of localtime but allowing the tool to support several timezones to respond to this reality. This was even clearer when we found out usecases that needed to express times relative to the "localtime where a supply point is installed" (to create appointment times required in some site works, for example).
Another place with discussion is a thread where you are involved and where there is an intention differenciating between "localtime" and "kraken localtime":
I think the naming is the key and we should talk about "kraken time" as a concretion of localtime the same way industrytime is a concretion of localtime. On the other side, localtime should be a module that provide an generic interface with wrappers and helpers for date and datetimes but should not be tied to any timezone at all.
There was a problem hiding this comment.
"kraken time" as a concretion of localtime the same way industrytime is a concretion of localtime
I think this is where we disagree: as I understand it, localtime is a specific timezone (the Kraken instance's "home" time zone), 'industrytime' is another specific timezone. Both of these are concretizations of generic timezone-aware functions. At the moment, those "generic" functions are all in the localtime module with defaults that use the "home" time zone. I think we should separate localtime from the generic functions it is based on, so in kraken-core it looks a lot more like the industrytime module.
Remember this repo isn't Kraken, this is a set of generic tools we've open sourced, so it needs to make sense outside the context of kraken-core.
Also worth being aware that having a single "industry" timezone is something we know doesn't necessarily make sense: we have clients in places where there will be multipel industry time zones in different industries/markets, so we already know we need to be able to expand on a generic base of functions.
Given that sounds like a job you aren't prepared to take on right now (and fair enough), I'm not blocking this change.
| @@ -60,12 +60,15 @@ def datetime( | |||
| minute: int = 0, | |||
| second: int = 0, | |||
| microsecond: int = 0, | |||
| tz: timezone.zoneinfo.ZoneInfo | None = None, | |||
There was a problem hiding this comment.
| tz: timezone.zoneinfo.ZoneInfo | None = None, | |
| tz: zoneinfo.ZoneInfo | None = None, |
Due to some refactoring, the leading timezone is causing mypy indigestion. We get the type directly from the zoneinfo stdlib now.
What I did
tzargument to the functionsmake_aware_assuming_localanddatetime, so these functions can use the argument as base timezone to perform the actions (instead of the default timezone).Why I did it
localtimeand wrap it to use it with a different timezone. While functions likemidnightortomorrowcan be easily adjusted to be executed with a custom timezone, the ones modified in this PR did not offer this possibility.