search

JuliaTime / TimeZones.jl

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

replace EzXML dependency by regex parsing #329

Closed laborg closed 3 years ago

laborg commented 3 years ago

This PR replaces the XML parsing by using a simple regex leading to the removal of EzXML as a dependency. I understand that that usually regex parsing of XML files is a nogo, but in this case the sources files seem stable enough. Based on data from JuliaHub, TimeZones.jl is an indirect dependency of 315 packages, and getting rid of EzXML might be benefitial for those packages.

codecov-io commented 3 years ago

Codecov Report

Merging #329 (9d68010) into master (906cea0) will decrease coverage by 3.27%. The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
- Coverage   93.80%   90.53%   -3.28%     
==========================================
  Files          32       32              
  Lines        1534     1162     -372     
==========================================
- Hits         1439     1052     -387     
- Misses         95      110      +15
Impacted Files Coverage Δ
src/winzone/WindowsTimeZoneIDs.jl 0.00% <0.00%> (-79.17%) :arrow_down:
src/compat.jl 0.00% <0.00%> (-100.00%) :arrow_down:
src/types/variabletimezone.jl 77.77% <0.00%> (-22.23%) :arrow_down:
src/tzdata/timeoffset.jl 82.14% <0.00%> (-11.98%) :arrow_down:
src/types/fixedtimezone.jl 90.00% <0.00%> (-10.00%) :arrow_down:
src/utcoffset.jl 90.90% <0.00%> (-9.10%) :arrow_down:
src/arithmetic.jl 84.21% <0.00%> (-7.79%) :arrow_down:
src/tzdata/download.jl 90.24% <0.00%> (-5.68%) :arrow_down:
src/class.jl 93.54% <0.00%> (-3.68%) :arrow_down:
... and 20 more

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 906cea0...9d68010. Read the comment docs.

oxinabox commented 3 years ago

Is this a real issue you are facing right now, or just a theoretical one? (Not that it isn't valid if theoretical, but it lends a weight of exerience and also a kind of time pressure on fixing if it is affecting you right now)

laborg commented 3 years ago

First off: Thanks for working on such an essential package! For me the additional dependency its just a theoretical problem (I was skimming through common Julia packages and have seen a couple of them having non-intuitive dependencies). Due to the popularity of TimeZones.jl it seems right to be picky on adding dependencies.

omus commented 3 years ago

It looks like the docs Manifest.toml needs to be updated before merging. I'll try to do that tomorrow

omus commented 3 years ago

I've moved these changes to https://github.com/JuliaTime/TimeZones.jl/pull/352 which is a branch I can modify