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

Add tz argument to some localtime functions #88

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gosku
Copy link

@gosku gosku commented Sep 1, 2023

What I did

  • I added an optional tz argument to the functions make_aware_assuming_local and datetime, so these functions can use the argument as base timezone to perform the actions (instead of the default timezone).

Why I did it

  • There are other libraries that use localtime and wrap it to use it with a different timezone. While functions like midnight or tomorrow can be easily adjusted to be executed with a custom timezone, the ones modified in this PR did not offer this possibility.

@gosku gosku marked this pull request as ready for review September 1, 2023 04:52
Copy link
Contributor

@samueljsb samueljsb left a comment

Choose a reason for hiding this comment

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

I'm a bit confused by this change -- if we pass in a different timezone, we aren't asking about local time, so why would we be using the local time module at all?

) -> 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.
Copy link
Contributor

@samueljsb samueljsb Sep 5, 2023

Choose a reason for hiding this comment

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

❓ 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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

@gosku gosku Sep 11, 2023

Choose a reason for hiding this comment

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

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":

https://octoenergy.slack.com/archives/C01ECAV4CHX/p1669293789295749?thread_ts=1669291784.132449&cid=C01ECAV4CHX

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

"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.

Copy link

@leamingrad leamingrad left a comment

Choose a reason for hiding this comment

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

I think these changes are sensible given where the module currently is, so I'm approving.

Copy link
Contributor

@pydanny pydanny 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, just needs one small change so it will merge properly.

@@ -60,12 +60,15 @@ def datetime(
minute: int = 0,
second: int = 0,
microsecond: int = 0,
tz: timezone.zoneinfo.ZoneInfo | None = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

@jamesbeith jamesbeith removed their request for review August 1, 2024 23:17
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