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

Add support for new serialization format TZJFile #380

Merged
merged 14 commits into from
Jun 20, 2022
Merged

Add support for new serialization format TZJFile #380

merged 14 commits into from
Jun 20, 2022

Conversation

omus
Copy link
Member

@omus omus commented Jun 17, 2022

Part of #359. From the lessons learned from #379 it was determined that we couldn't use the tzfile format for storing the TimeZones.jl internal representation and instead we needed a custom binary format mostly based upon the existing tzfile format.

This PR implements this new format as well as switches the time zone building and loading to utilize this new format. I've opt'd to have the read/write capabilities be a part of TimeZones.jl rather than push this into a new package as this code is just reading/writing the format into TimeZones.jl specific types.

@omus omus self-assigned this Jun 17, 2022
@omus
Copy link
Member Author

omus commented Jun 17, 2022

Mainly just need to write some tests for this

@omus omus marked this pull request as ready for review June 17, 2022 19:33
@omus
Copy link
Member Author

omus commented Jun 17, 2022

Demonstration of the new tzjfile format in action. As our internal representation hasn't changed we can still use the TZData.compile function to go straight from tzsource files into the TimeZones representation. If we then do a round-trip through the tzjfile format we can identify any incompatibilities:

julia> using TimeZones

julia> using TimeZones: TZData, TZJFile

julia> tz_source = TZData.TZSource(joinpath.(TZData.TZ_SOURCE_DIR, readdir(TZData.TZ_SOURCE_DIR)));

julia> compiled = TZData.compile(tz_source)
595-element Vector{Tuple{TimeZone, TimeZones.Class}}:
 (tz"Europe/Zaporozhye", TimeZones.Class(:STANDARD))
 (tz"Australia/Lindeman", TimeZones.Class(:STANDARD))
 (tz"Europe/Kaliningrad", TimeZones.Class(:STANDARD))
 (tz"Pacific/Nauru", TimeZones.Class(:STANDARD))
 
 (tz"Europe/Guernsey", TimeZones.Class(:STANDARD))
 (tz"Australia/Tasmania", TimeZones.Class(:LEGACY))
 (tz"America/Santa_Isabel", TimeZones.Class(:LEGACY))
 (tz"Africa/Banjul", TimeZones.Class(:STANDARD))

julia> function convert_to_tzjf(tz, class)
           io = IOBuffer()
           TZJFile.write(seekstart(io), tz; class)
           return TZJFile.read(seekstart(io))(tz.name)
       end
convert_to_tzjf (generic function with 1 method)

julia> tzjf_compatible((tz, class)) = convert_to_tzjf(tz, class) == (tz, class)
tzjf_compatible (generic function with 1 method)

julia> filter(!tzjf_compatible, compiled)
Tuple{TimeZone, TimeZones.Class}[]

Base automatically changed from cv/write_tzfile to master June 20, 2022 20:11
@omus
Copy link
Member Author

omus commented Jun 20, 2022

Part of the issue with #359 is that the TimeZones.jl build step isn't always re-triggered. It seems prudent to avoid switching the internal format used until we bypass the build system altogether. I'll remove some of the current logic in this PR such that this PR only adds support for the TZJFile format but does switch to using it as the default.

@omus omus changed the title Use new serialization format TZJFile Add support for new serialization format TZJFile Jun 20, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #380 (f382a5e) into master (c19864a) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #380      +/-   ##
==========================================
+ Coverage   94.48%   94.80%   +0.32%     
==========================================
  Files          32       35       +3     
  Lines        1649     1753     +104     
==========================================
+ Hits         1558     1662     +104     
  Misses         91       91              
Impacted Files Coverage Δ
src/TimeZones.jl 100.00% <ø> (ø)
src/tzjfile/read.jl 100.00% <100.00%> (ø)
src/tzjfile/utils.jl 100.00% <100.00%> (ø)
src/tzjfile/write.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c19864a...f382a5e. Read the comment docs.

@omus omus merged commit beeeb0a into master Jun 20, 2022
@omus omus deleted the cv/tzjfile branch June 20, 2022 20:54
kpamnany pushed a commit to RelationalAI-oss/TimeZones.jl that referenced this pull request May 5, 2023
* Sync up with TZFile changes

* Write tests

* Update TZJFile to use get_designation

* Add comment for future version test
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.

2 participants