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

move from CLDR artifact to code generation #450

Merged
merged 12 commits into from
Apr 26, 2024
Merged

move from CLDR artifact to code generation #450

merged 12 commits into from
Apr 26, 2024

Conversation

visr
Copy link
Contributor

@visr visr commented Oct 24, 2023

Fixes #373, based on the proposal outlined in #373 (comment). 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 added a commit to Deltares/Ribasim that referenced this pull request Oct 25, 2023
Alternative to #687, which wasn't working well with PackageCompiler due to long paths still.

This switches to a fork. A PR to merge this upstream has already been submitted: JuliaTime/TimeZones.jl#450

Clearly this is temporary, but since we use Manifests this should work ok.
Hofer-Julian pushed a commit to Deltares/Ribasim that referenced this pull request Oct 25, 2023
Alternative to #687, which wasn't working well with PackageCompiler due
to long paths still, leading to broken builds.

This switches to a fork. A PR to merge this upstream has already been
submitted: JuliaTime/TimeZones.jl#450

Clearly this is temporary, but since we use Manifests this should work
ok.
visr added a commit to Deltares/Ribasim that referenced this pull request Oct 25, 2023
Alternative to #687, which wasn't working well with PackageCompiler due
to long paths still, leading to broken builds.

This switches to a fork. A PR to merge this upstream has already been
submitted: JuliaTime/TimeZones.jl#450

Clearly this is temporary, but since we use Manifests this should work
ok.
@visr
Copy link
Contributor Author

visr commented Nov 14, 2023

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

@omus
Copy link
Member

omus commented Nov 30, 2023

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.

Co-authored-by: Curtis Vogt <[email protected]>
@visr
Copy link
Contributor Author

visr commented Dec 1, 2023

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
Copy link
Contributor Author

visr commented Feb 10, 2024

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

@visr
Copy link
Contributor Author

visr commented Apr 10, 2024

Bump :)

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I'm good with proceeding with this change once we complete a round of refactoring. It would be good to make the separate repo but I'm good to adopt this here first.

dev/generate_windows_zones.jl Outdated Show resolved Hide resolved
dev/generate_windows_zones.jl Outdated Show resolved Hide resolved
dev/generate_windows_zones.jl Outdated Show resolved Hide resolved
dev/generate_windows_zones.jl Outdated Show resolved Hide resolved
dev/generate_windows_zones.jl Outdated Show resolved Hide resolved
dev/generate_windows_zones.jl Outdated Show resolved Hide resolved
dev/generate_windows_zones.jl Outdated Show resolved Hide resolved
src/windows_zones.jl Outdated Show resolved Hide resolved
test/windows_zones.jl Show resolved Hide resolved
test/windows_zones.jl Outdated Show resolved Hide resolved
@visr
Copy link
Contributor Author

visr commented Apr 26, 2024

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.

Copy link
Member

@omus omus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just needs the io::IO change and it looks like the benchmark Project.toml may need some updating. We'll see what CI says about that on this run.

dev/generate_windows_zones.jl Outdated Show resolved Hide resolved
test/windows_zones.jl Show resolved Hide resolved
@visr
Copy link
Contributor Author

visr commented Apr 26, 2024

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 #446 (comment).

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@omus omus merged commit 7448c09 into JuliaTime:master Apr 26, 2024
14 of 15 checks passed
@visr visr deleted the cldr branch April 27, 2024 06:37
visr added a commit to Deltares/Ribasim that referenced this pull request Apr 29, 2024
Since [our
contribution](JuliaTime/TimeZones.jl#450) is
merged upstream and released, we can stop using a fork.
We don't directly depend on this package, but it is an Arrow.jl
dependency. Therefore it can now be removed as a direct dependency,
which was only done to be able to specify compat and point it to a fork.
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

Successfully merging this pull request may close these issues.

Reduce Unicode CLDR artifact size
3 participants