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

Consider new major for each tzdata version #31

Open
green-nsk opened this issue Sep 10, 2024 · 6 comments
Open

Consider new major for each tzdata version #31

green-nsk opened this issue Sep 10, 2024 · 6 comments

Comments

@green-nsk
Copy link

Recent TZJData release broke our code. tz"CET" now throws because CET timezone has been deprecated by IANA recently. Our CI process caught the issue, but there's no reasonable workaround because 1) we don't depend directly on TZJData, only via TimeZones.jl 2) this is only a minor release, so package manager doesn't think twice about pulling it.

Please consider making each release a major release s.t. semantic versioning works as expected.

TimeZones.jl should consider doing the same, though I guess it's less clear-cut and an issue for that package anyway.

@green-nsk green-nsk changed the title Consider new major for each version Consider new major for each tzdata version Sep 10, 2024
@omus
Copy link
Member

omus commented Sep 10, 2024

but there's no reasonable workaround

Available options without changing the semver setup:

  1. Use TimeZone("CET", TimeZones.Class(:ALL)) which lets you to continue to use legacy time zones
  2. Use the non-deprecated name for "CET" which is "Europe/Brussels". I need to make the replacement names more easily discoverable: Provide user friendly exceptions with deprecated time zones TimeZones.jl#469 (Note: == will fail as they have different names. The transitions are equivalent though)
  3. Add the dependency on TZJData.jl and set your compat appropriately (e.g. TZJData = "~1.2")

Please consider making each release a major release s.t. semantic versioning works as expected.

Deprecations are commonly included in minor version bumps for Julia packages. The difference here is that TimeZones.jl doesn't allow the use of legacy/deprecated time zones by default. I could see an argument to be made for tying the default TimeZone.Class with the --depwarn flag and having --depwarn=error be the only mode where TimeZone.Class(:LEGACY) is disallowed.

@green-nsk
Copy link
Author

Add the dependency on TZJData.jl and set your compat appropriately (e.g. TZJData = "~1.2")

Indeed that would help, made me re-read how ~compat works. Knowing to add a TZJData dependency just for semver is not obvious though, we don't directly depend on it. TBH I wasn't even aware that TImeZones and TZData are separate packages until yesterday.

We went with replacing CET for more specific timezone as you suggested, seems like we'd have to do this anyway at some point.

I could see an argument to be made for tying the default TimeZone.Class with the --depwarn flag and having --depwarn=error be the only mode where TimeZone.Class(:LEGACY) is disallowed.

I'm unfamiliar with the information in TZJData and what changes can be made between the releases. Could the next tzdata version entirely remove CET or some other time zones? If there's no certainty about this, I would err on the cautious side, that's why I suggested major versions for each new release.

If we know that timezones are never deleted completely, what you suggested sounds appropriate.

@omus
Copy link
Member

omus commented Sep 11, 2024

I'm unfamiliar with the information in TZJData and what changes can be made between the releases. Could the next tzdata version entirely remove CET or some other time zones? If there's no certainty about this, I would err on the cautious side, that's why I suggested major versions for each new release.

If we know that timezones are never deleted completely, what you suggested sounds appropriate.

That's a good point. I think a good compromise in this situation would be to add a test which ensures that the new release includes all of the time zone names of the previous release. This would enforce that each new release is a supserset of the old release. I agree if these deprecated time zones were removed we should make a breaking TZJData.jl release.

One aspect where this gets tricky is that TimeZones.jl wouldn't really care that time zones were dropped so users would still need a TZJData.jl compat entry for this to be effective against breakage.

@ericphanson
Copy link

ericphanson commented Nov 9, 2024

if we want TZJData to be transparent to the user, one thing TimeZones.jl could do is strictly compat bound TZJData itself, and include in its public API e.g. what tz"CET" does. To me that would make sense. It would mean we would need a new TimeZones.jl release for each TZJData one though, but maybe some of that could be automated (e.g. gen/make.jl could PR a version bump or something).

I think an issue is that there's kind of two user stories for TimeZones:

  • I want to perform date operations on timezone aware dates, or store them, or pass them around
    • I probably don't want to have to make frequent breaking bumps to my TimeZones.jl deps (potentially deep in a stack) due to changes in timezone definitions, since my code just passes through timezone handling
  • I want to construct ZonedDateTimes, like tz"CET"
    • I probably do want compat bounds to ensure this continues to work in non-breaking releases

I think having TZJData have separate compat bounds allows those use cases to both work, but it means users in the second case need to know about TZJData and compat bound it.

@DilumAluthge
Copy link

I want to perform date operations on timezone aware dates, or store them, or pass them around

  • I probably don't want to have to make frequent breaking bumps to my TimeZones.jl deps (potentially deep in a stack) due to changes in timezone definitions, since my code just passes through timezone handling

My suspicion is that the majority of users fall into this category.

@omus
Copy link
Member

omus commented Nov 12, 2024

if we want TZJData to be transparent to the user, one thing TimeZones.jl could do is strictly compat bound TZJData itself, and include in its public API e.g. what tz"CET" does. To me that would make sense. It would mean we would need a new TimeZones.jl release for each TZJData one though, but maybe some of that could be automated (e.g. gen/make.jl could PR a version bump or something).

Part of the reason for the TZJData.jl separation from TimeZones.jl was to allow using different tzdata versions while keeping the behaviour of TimeZones.jl the same. If we would have the strict compat bounds like you're suggesting we'd be better off just returning to having an Artifacts.toml in TimeZones.jl directly.

I think a better option is to introduce some tests/utilities which can compare the tzdata class changes between versions. With that we should be able to come up with some rules for what kind of changes designate a major tzdata version release versions a minor release. Off the top of my head I think that a deprecation of tz"CET" would be a minor change as long as users can still use it with a deprecation notice (JuliaTime/TimeZones.jl#469 (comment)). In the event a time zone is actually removed then we should make a breaking TZJData.jl release. I've found that tzdata rarely removes time zones so I expect this would be quite rare.

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

No branches or pull requests

4 participants