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

Mapping revision not correctly injected in tzdb2020a.nzd #1559

Open
mnivet opened this issue Jul 3, 2020 · 10 comments
Open

Mapping revision not correctly injected in tzdb2020a.nzd #1559

mnivet opened this issue Jul 3, 2020 · 10 comments
Assignees
Labels

Comments

@mnivet
Copy link

mnivet commented Jul 3, 2020

The revision number seem to have been badly injected in this file
This is visible in the VersionId property of an IDateTimeZoneProvider loaded from that file which is "TZDB: 2020a (mapping: $Revision$)" while a number would be expected instead of that $Revision$
This is also visible on that page: https://nodatime.org/TimeZones?version=2020a#

It' not a big deal but it's just visually annoying for me.

@jskeet
Copy link
Member

jskeet commented Jul 3, 2020

Ooh, that's odd. Thanks, will fix. I'll probably leave the 2020a file as it is, but get it fixed for 2020b. No idea what's gone wrong there... it looks like it was okay for 2019c.

@jskeet jskeet added the bug label Jul 3, 2020
@jskeet jskeet self-assigned this Jul 3, 2020
@jskeet
Copy link
Member

jskeet commented Jul 3, 2020

It turns out this is a CLDR problem that we're faithfully propagating. CLDR v36 has this in its mapping file:

<version number="$Revision$"/>

Currently seeing if CLDR v37 has the same problem...

@jskeet
Copy link
Member

jskeet commented Jul 3, 2020

Nope, CLDR v37 is the same. Will leave this issue open, but I'm not sure of the best approach.

I've filed https://unicode-org.atlassian.net/browse/CLDR-13931 to see if this can be fixed for v38...

jskeet added a commit to jskeet/nodatime that referenced this issue Jul 3, 2020
Note that this still has the same problem as v36 in terms of the
revision, as noted in nodatime#1559.
jskeet added a commit that referenced this issue Jul 3, 2020
Note that this still has the same problem as v36 in terms of the
revision, as noted in #1559.
@torbenj
Copy link

torbenj commented Apr 14, 2021

Is the $Revision$ value mandatory when using Tzdb or can I just use TzdbDateTimeZoneSource.Default.TzdbVersion (which currently gives me 2021a), i.e. are there revisions of 2021a (or any future version) or will the next be different e.g. 2021b?

@jnyrup
Copy link
Contributor

jnyrup commented Oct 29, 2021

Just wanted to chime in with an observation I made while looking through the diffs of CLDR-40
https://raw.githubusercontent.com/unicode-org/cldr/release-39/common/supplemental/windowsZones.xml
https://raw.githubusercontent.com/unicode-org/cldr/release-40/common/supplemental/windowsZones.xml

Both files have <version number="$Revision$"/> and <mapTimezones otherVersion="7e11800" typeVersion="2021a">.
Directory.GetFiles doesn't guarantee an order.
So in LoadWindowsZones allFiles could have CLDR-39 or CLDR-40 as the newest item.

On my machine the files are sorted as follows

[0]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-39.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[1]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-40.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[2]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-38-1.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[3]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-38.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[4]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-36.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[5]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-37.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[6]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-35.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[7]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-33.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[8]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-31.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[9]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-32.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[10]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-30.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[11]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-29.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[12]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-28.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[13]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-27.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[14]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-26.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[15]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-25.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[16]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-24.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[17]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-23.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[18]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-22-1.xml", {NodaTime.TimeZones.Cldr.WindowsZones})
[19]: ("C:\\dev\\nodatime\\data\\cldr\\windowsZones-21.xml", {NodaTime.TimeZones.Cldr.WindowsZones})

See that the order of (31, 32), (36, 37) and (39, 40) reversed compared to what one (I?) would expect.

@jskeet
Copy link
Member

jskeet commented Oct 29, 2021

Thanks, I'll have another look at this over the weekend. It may be that I need to artificially mess with the file :(

@jnyrup
Copy link
Contributor

jnyrup commented Oct 29, 2021

Would it suffice to sort by TzdbVersion and then by path?

@jskeet
Copy link
Member

jskeet commented Oct 29, 2021

Quite possibly - I just don't have time to look at what we're doing or what we should do right now.

@jskeet
Copy link
Member

jskeet commented Oct 29, 2021

Implemented in 142a0f7

But we still don't have the revision :(

@malcolmr
Copy link
Contributor

From https://unicode-org.atlassian.net/browse/CLDR-13931, it looked like they were planning on removing the version element entirely? (Although it appears to be present in all the CLDR files I can find, contrary to what that comment might suggest.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants