TimeZones.jl icon indicating copy to clipboard operation
TimeZones.jl copied to clipboard

move from CLDR artifact to code generation

Open visr opened this issue 2 years ago • 4 comments

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 avatar Oct 24 '23 23:10 visr

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

visr avatar Nov 14 '23 10:11 visr

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.

omus avatar Nov 30 '23 21:11 omus

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 avatar Dec 01 '23 04:12 visr

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

visr avatar Feb 10 '24 19:02 visr

Bump :)

visr avatar Apr 10 '24 19:04 visr

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 avatar Apr 26 '24 08:04 visr

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.

visr avatar Apr 26 '24 14:04 visr

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

codecov-commenter avatar Apr 26 '24 18:04 codecov-commenter