search

JuliaTime / TimeZones.jl

IANA time zone database access for the Julia programming language
Other
86 stars 51 forks source link

move from CLDR artifact to code generation #450

Closed visr closed 2 months ago

visr commented 8 months ago

Fixes #373, based on the proposal outlined in https://github.com/JuliaTime/TimeZones.jl/issues/373#issuecomment-1777825007. The main benefit is to remove a large artifact download on Windows, which has long paths, leading to issues with PackageCompiler.

This removes the WindowsTimeZoneIDs module. Even though the generated dict is only used in Windows, I decided to remove the conditional Sys.iswindows includes since these translations could perhaps be useful on other platforms as well.

visr commented 8 months ago

@omus I would by happy to hear if you think this is a good way forward.

omus commented 7 months ago

Sorry, I've been rather swamped lately.

I only have one major concern with this approach and is that each time there is a CLDR release we need to update TimeZones.jl. The issue with that is some users can end up in a situation where they may not want to update to the latest version of TimeZones.jl but do want to have the latest CLDR release. My personal preference would be to make a package akin to TZJData.jl which we can update independently from the TimeZones.jl code.

With that said the rest of the approach does seem reasonable and maybe it's best to adopt this and break this into a separate package later.

visr commented 7 months ago

Thanks. Indeed I can see a benefit of having a separately upgradable package for this. I went for this change within the package to keep the PR somewhat small. I figured that when switching to a separate package we'd want to generate the code like this as well, so it is a good step to take regardless.

If you prefer I'd be happy to create a separate package.

I looked at some past cldr releases and saw that the Windows time zone IDs only change every few cldr releases.

visr commented 5 months ago

Would love to hear what I can do to land this.

visr commented 3 months ago

Bump :)

visr commented 2 months ago

Thanks for the review! I completed the refactoring and regenerated using the latest CLRD release. Only left main(ARGS) in, let me know if you prefer to change that as well.

Could you approve the CI workflow? Locally on Windows it passes all tests.

visr commented 2 months ago

looks like the benchmark Project.toml may need some updating

Yes, I see "Package TimeZones does not have Artifacts in its dependencies". This looks like it is due to origin/HEAD needing Artifacts whereas this removes it. Same happened in https://github.com/JuliaTime/TimeZones.jl/pull/446#issuecomment-1688605979.

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.60%. Comparing base (b20904c) to head (d27bd94). Report is 2 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #450 +/- ## ========================================== - Coverage 92.95% 91.60% -1.36% ========================================== Files 40 37 -3 Lines 1845 1667 -178 ========================================== - Hits 1715 1527 -188 - Misses 130 140 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.