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.

Sign up for GitHub

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
Open

Add tz argument to some localtime functions #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71bee5c
Add optional tz argument to make_aware function
gosku Sep 1, 2023
9ef63c3
Add optional tz argument to localtime.datetime
gosku Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions tests/test_localtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,23 @@ def test_seconds_in_past(self):
)


class TestMakeAwareAssumingLocal:
def test_assume_default_timezone_when_none_provided(self):
dt = datetime.datetime(2022, 1, 1, 0)

timezone_aware_dt = localtime.make_aware_assuming_local(dt)

assert timezone_aware_dt.tzinfo == timezone.get_current_timezone()

def test_make_aware_with_provided_timezone(self):
dt = datetime.datetime(2022, 1, 1, 0)
tz = zoneinfo.ZoneInfo("Etc/GMT-10")

timezone_aware_dt = localtime.make_aware_assuming_local(dt, tz=tz)

assert timezone_aware_dt.tzinfo == tz


class TestDate:
def test_date_calculation_near_midnight_during_bst(self):
near_midnight_in_utc = datetime.datetime(2016, 6, 1, 23, 50, 0, tzinfo=localtime.UTC)
Expand Down Expand Up @@ -299,6 +316,13 @@ def test_dst_ambiguity(self):
utc_dt = dt.astimezone(localtime.UTC)
assert utc_dt.hour == 0

def test_datetime_with_specific_timezone(self):
aus_time = zoneinfo.ZoneInfo("Etc/GMT-10")
dt = localtime.datetime(2016, 8, 5, tz=aus_time)
assert dt.hour == 0
utc_dt = dt.astimezone(localtime.UTC)
assert utc_dt.hour == 14


class TestAsLocaltime:
def test_conversion_of_gmt_dt(self):
Expand Down
19 changes: 13 additions & 6 deletions xocto/localtime.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

) -> 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 •
edited
Loading

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 •
edited
Loading

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.

"""
if tz is None:
tz = timezone.get_current_timezone()
dt = datetime_.datetime(year, month, day, hour, minute, second, microsecond)
return timezone.make_aware(dt)
return timezone.make_aware(dt, timezone=tz)


# Returning dates
Expand Down Expand Up @@ -349,13 +352,17 @@ def as_range(
return midnight(_date, tz), latest(_date, tz)


def make_aware_assuming_local(dt: datetime_.datetime) -> datetime_.datetime:
def make_aware_assuming_local(
dt: datetime_.datetime, tz: Optional[datetime_.tzinfo] = None
) -> datetime_.datetime:
"""
Just a wrapper for Django's method, which will takes a naive datetime, and makes it timezone
aware, assuming the current timezone if none is passed (which it isn't from this wrapper
function). It will also raise an exception if the passed datetime is already timezone-aware.
aware, assuming the current timezone if none is passed.
It will also raise an exception if the passed datetime is already timezone-aware.
"""
return timezone.make_aware(dt)
if tz is None:
tz = timezone.get_current_timezone()
return timezone.make_aware(dt, timezone=tz)


def make_aware_assuming_utc(dt: datetime_.datetime) -> datetime_.datetime:
Expand Down