add isdst to moment tz pack/unpack files #2
Conversation
(otherwise they don't run with :latest)
so isdst is contained in unpacked files
so isdst is contained in unpacked files
note ... since the moment isdst calculation and isdst from tzdb do not necessarily agree the tests might fail :/
so isdst is contained in packed files
|
@pithu .. .could you have a look at this? it's a first step for the dst into timezone and locale ... for some reason i can't run the data:2021e target ... data:2021a and data:latest works fine... i tried to split the commits, there is two commits that update the generated test and data files ... the others are code changes |
|
there will be more PRs coming for joda timezone and locale, but since they are depending on this, i'd like to get this done first |
- meta needs to run before collect
|
ok, managed to get the 2021e target running... for some reason the grunt tasks order was off... well ... i guess this should work now ... question is how we proceed... we could leave it in a branch here and always use this branch to update tz data files.. or merge it to develop here and use that branch, and just pull from upstream from time to time ... or we try to get the change upstream but i'm not sure if they'll take it if they don't need the infos :/ |
Good question, once i made a pr to upstream with a tz data update and got no response at all. But we could at least try (let them know) even if the chance is probably zero and moment is not developed anymore anyway. I would guess the best would be probably to merge it to |
| packAbbrsAndOffsets(source), // 1 - abbrs, 2 - offsets, 3 - indices | ||
| packUntils(source.untils), // 4 - untils | ||
| packPopulation(source.population) // 5 - population | ||
| packPopulation(source.population), // 5 - population |
There was a problem hiding this comment.
btw: Do you know what the population is required for ?
There was a problem hiding this comment.
Should we simply skip it ? We don't need it, will save some space and i guess we will never push this upstream. So why not have a custom js-joda format ?
|
|
||
| return out.join(''); | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder if there is more compact way to store isDst ? Isn't dst just a flag 0 or 1 ? Eg take it as binary, prefix it with 1 and then convert it to an hex format or use this base60 converter. The current implementation increase file size of 2021e by 14%, there might be space for optimisation
There was a problem hiding this comment.
zdump man page says:
Each line is followed by isdst=D where D is positive, zero, or negative depending on whether the given time is daylight saving time, standard time, or an unknown time type
so while currently it is only 0 and 1, in theory it could be any number, thus i didn't know how to decrease the size really ... but yes, we currently need one byte for each dst info ... that might be overkill :/
There was a problem hiding this comment.
Interesting problem, we can solve it any time later as long as we have corresponding packer/ unpacker.
If I get it right isdst is a flag with 3 states saving, standard or unknown. That should be packable somehow.... An array of a 3 states flag...
There was a problem hiding this comment.
I found some time to pay around with a packing algorithm. see branch https://github.com/js-joda/js-joda/commits/isdst-pack. The 3 state approach compact ratio is about 1/3. But practically the unknown state is probably without use, the only thing you can do there is to show a non dst case, i guess. So the last commit contains a 2 state approach and compact about 18%.
2 participants
add isdst to moment-timezone files
we need the isdst information for DateTimeFormatter.ofPattern('zzz') does not include daylight/standard time indicator js-joda#518
add isdst to the data files, tests, unpacked and packed files