regebro / tzlocal

A Python module that tries to figure out what your local timezone is
MIT License
185 stars 58 forks source link

Fixing the mess 3.0 put me in #117

Closed regebro closed 2 years ago

regebro commented 3 years ago

It's become clear to me that 3.0 was a mistake, I should not have replaced pytz with zoneinfo, at least not in one go, a slower deprecation would have been better. Or maybe a fork. I lost track of why this library exist in the first place, I'm sorry about that.

Why tzlocal exists

tzlocal exists because there was no way to get the local timezone with pytz. In dateutil, you could, it had a method for that, with pytz, you HAD to know the local timezone name. So I made this library to get the local timezone name. However, I realized that some Unix installations did not have a local timezone configured, instead you had to use the /etc/localtime file. In that case, you couldn't figure out the name, you could only return the anonymous timezone.

So, tzlocal returned pytz timezone objects, to deal with that.

The mess

Then came zoneinfo, and @agronholm needed a zoneinfo version of tzlocal. And I thought "Well, people are going to stop using pytz over time, so...."

However... on Unix (haven't tried OS X yet) you can do zoneinfo.ZoneInfo('localtime'). So you don't actually need tzlocal at all. And on Windows you can always find a zoneinfo name, so you don't need tzlocal to return an object, just the zoneinfo name.

So switching tzlocal to return ZoneInfo objects actually never made sense. But that's what happened. So the question now, is how to get out of the mess. And there are a few ways:

Option 1

Option 2

Option 3

Option 4

Option 5

Option 6

Please discuss!

agronholm commented 3 years ago

Then came zoneinfo, and @agronholm needed a zoneinfo version of tzlocal. And I thought "Well, people are going to stop using pytz over time, so...."

However... on Unix (haven't tried OS X yet) you can do zoneinfo.ZoneInfo('localtime'). So you don't actually need tzlocal at all. And on Windows you can always find a zoneinfo name, so you don't need tzlocal to return an object, just the zoneinfo name.

Well, this is news to me. But I tried that on both Linux and Windows and got zoneinfo._common.ZoneInfoNotFoundError: 'No time zone found with key localtime'. The stdlib documentation also does not mention this, nor does the documentation of the backport. So where is this coming from?

agronholm commented 3 years ago

Are these downstream users missing some key functionality now with pytz support gone? Or is the problem just that they haven't adjusted to the ZoneInfo API?

regebro commented 3 years ago

It works on the Linuxes I tried, which admittedly aren't that many.

But if it doesn't work on yours, that does make some of these options non-compelling.

>>> import zoneinfo
>>> ltz = zoneinfo.ZoneInfo('localtime')
>>> ltz
zoneinfo.ZoneInfo(key='localtime')
>>> import datetime
>>> ltz.tzname(datetime.datetime.now())
'CEST'

"Are these downstream users missing some key functionality now with pytz support gone?"

The problem is just that they need to switch from pytz, that can be a lot of work for some, but sure, sooner or later they probably need to do it.

agronholm commented 3 years ago

The problem is just that they need to switch from pytz, that can be a lot of work for some, but sure, sooner or later they probably need to do it.

If that's the only issue, then they just need to pin tzlocal to 2.X, and move on when they've adjusted their code. That's what I did with APScheduler.

Were you really not expecting this when you made a backwards incompatible API change?

regebro commented 3 years ago

Yes, but I could have avoided that in hindsight. And handling the localtime instance is better done in other ways, so doing it this way was a mistake in tzlocal from the start.

And the issue is how to move forward, not that I'm an idiot, I already know that.

agronholm commented 3 years ago

Well IMHO, pytz needs to die. I don't really see any need for changes in tzlocal. Downstream code breaking is a natural consequence of this and the developers will need to adjust. If they don't follow tzlocal on libraries.io or similar services, then I doubt there would have been any way to communicate the upcoming breaking changes to them. But if you want to keep supporting pytz, you could add a keyword argument to get_localzone() that would then return a pytz timezone instead. Switching back to pytz timezones by default now would just spell an even greater disaster.

agronholm commented 3 years ago

In retrospect, this change could've been done in three phases: 1) allow ZoneInfo as an opt-in and deprecate get_localzone() without specifying the tz library, 2) make the tz library argument mandatory, 3) allow get_localzone() without the tz library argument but return ZoneInfo zones by default.

regebro commented 3 years ago

Yeah, I was thinking about adding an argument for which library to use, but that would be pretty messy, as they raise different errors and have different arguments.

But a function to get the local binary timezone data (from /etc/localtime), to pass in to the constructor of the timezone could be an option. Still not as smooth as getting the timezone object, though.

pganssle commented 3 years ago

I am on mobile right now, but I think what should have been done in the first place is some variation on using pytz-deprecation-shim, which generates shims around zoneinfo that mostly work like pytz zones.

Anyone who adapted to using zoneinfo should be mostly unaffected by this, and people who are still expecting pytz zones will get deprecation warnings.

I would recommend against advising people to use localtime or anything like that as an argument to zoneinfo.

FObersteiner commented 3 years ago

Just a quick comment. To me, the important feature of this package appears to be to get the IANA name of the local time zone, on multiple different platforms. Down the line, I can use that in what ever I need, zoneinfo, pytz, dateutil... I recently tried to do the same in go - inspired by this package here - and well, especially on Windows, I found this to be kind of a pain ;-)

agronholm commented 3 years ago

To me, the important feature of this package appears to be to get the IANA name of the local time zone, on multiple different platforms.

That could be made into a separate function which would then be used by get_localzone().

regebro commented 3 years ago

Yeah, I implemented get_localzone_name() yesterday. I'm currently leaning towards making a branch, switching it back to pytz and releasing that as 2.2, as the current main branch has some significant features 2.1 doesn't (something I also didn't anticipate for 3.0, I thought this package was going to stay stable).

regebro commented 3 years ago

pytz_deprecation_shim is interesting. I'll have to read up on that.

Also, should get_localzone_name() return None or error out if no name is found?

tacaswell commented 3 years ago

@regebro Thank you for taking this discussion in a very productive direction, not everyone would respond as well to the situation you are in :)


I agree with @pganssle , returning a wrapped zoneinfo object that warns (but does not fail) when accessed via the pytz API is the best path forward. In my case we had hard-failures in user-facing code that we had to scramble to sort out how to fix it (yes we should have seen it in CI sooner, yes we should have been more thoroughly pinning our production environments), a warning would have been much easier for us to deal with.

I would release a 4.x which returns the warning objects that are back-compatbile with pytz, but not worry about yanking 3.x. Anyone who has pinned to <3 will either be fine as they are or will need to adjust their pinning in the future anyway and can exclude 3.x or not as they chose. Anyone who is un-pinned at this point has either fixed the issue or not noticed yet. In the first case, they do not care if 3.x is yank (as they already adapted) and is the second case, getting a warning producing 4.x out ASAP will save them from hard-failures.

In general, "flag-day" API changes never work out well (you will never reach all of your users a head of time to let them know it is coming). Making sure both the old way (with warnings) and new way work at the same time for a while is critical so that users have a gentle path to making the change (particularly if users have tzlocal as both a direct dependency and a dependency of (multiple) dependencies).

tacaswell commented 3 years ago

As soon as I hit post, I thought a bit more and I think that releasing a 3.1 that warns on using the pytz API would be consistent with semantic versioning (it fixes a "bug" ;) ).

pganssle commented 3 years ago

Also, should get_localzone_name() return None or error out if no name is found?

I would recommend error, since almost certainly the consumers of such an API will be counting on a non-None value there.

regebro commented 2 years ago

New beta released.

agronholm commented 2 years ago

I still feel that it's dangerous to say

Python3.9's zoneinfo library supports ZoneInfo('localtime'), which is what tzlocal returns if it can't find a timezone name.

Since it does not seem to work on most operating systems and is not documented anywhere. I think it's a quirk of select Linux distros that actually have an alias for that.

regebro commented 2 years ago

Ah, I forgot to update the readme, as usual.

agronholm commented 2 years ago

I'm currently building a PR for APScheduler 3.x that enables compatibility with tzlocal versions beyond 2.x. It needs a pytz timezone, and my current patch works fine for well defined timezones. But what would I do if I got local from get_localzone_name()? Which operating systems return this value? Would pytz.timezone('localtime') do the trick on these systems?

FObersteiner commented 2 years ago

But what would I do if I got local from get_localzone_name()?

shouldn't the tz name string returned by get_localzone_name be restricted to IANA time zone names? (or raise an error in case not found)

agronholm commented 2 years ago

Then what am I supposed to do if it raises an error? Previously I could at least get a pytz compatible tzinfo object with tzlocal.get_localzone().

regebro commented 2 years ago

Yes, get_localzone_name should fail in those cases where you can get an unnamed "local" timezone object.

With pytzshim in the picture, I'll keep get_localzone() if you want an object, and have get_localzone_name() if you want the timezone name.

agronholm commented 2 years ago

With pytzshim in the picture, I'll keep get_localzone() if you want an object, and have get_localzone_name() if you want the timezone name.

So...does that mean get_localzone() will give me a pytz timezone again?

agronholm commented 2 years ago

Never mind; I found the shim package.

So is the plan to just add a new function and keep everything else as-is? Then the major version bump would be unnecessary.

regebro commented 2 years ago

@agronholm No, it will give you a pytz_shim timezone, which should be compatible with both the pytz API and the zoneinfo API (with one known exception, it doesn't have a .key attribute, which zoneinfo zones have. But str(tz) works for both.

The major version bump is not just the shim, but also a refactoring.

regebro commented 2 years ago

4.0 released, so time to close this. Thanks for the feedback!