Fix for issue# 15376. Get time zone conversions at runtime. #3824
Conversation
|
|
||
| -------------------- | ||
| auto text = std.file.readText("path/to/windowsZones.xml"); | ||
| auto conversions = parseTZConversions(std.file.readText(text)); |
There was a problem hiding this comment.
Maybe suggest parseTZConversions(std.net.curl.get(url_from_above)) as well, and you have readText twice in your example.
jmdavis
force-pushed
the
issue15376
branch
2 times, most recently
from
759fbda to
c009bef
Compare
|
I am a bit concerned that the ad-hoc XML "parsing" might break with future versions of the file, e.g. due to new time zone names containing XML entities. Maybe it'd be worth using std.xml (as bad as it is)? |
I considered it, but it looked like it was going to be a pain to rewrite the code to use it. If you folks reviewing the PR insist on it, I can rewrite it, but I'd just as soon not deal with std.xml. And if we do merge the PR as-is, someone can always rewrite it to use std.xml if they really want to, since it's an implementation detail. |
| line = line[f1.length .. $]; | ||
| auto next = line.find('"'); | ||
| enforce(!next.empty, "Error parsing. Text does not appear to be from windowsZones.xml"); | ||
| auto win = to!string(line[0 .. $ - next.length]); |
There was a problem hiding this comment.
to!string
Why? Is it to make a copy?
There was a problem hiding this comment.
to!string does not make a copy if the original type is string, only if it's a type which needs to be converted to string. Rather, if to!string is given a string, it just returns it, and all it costs you is the overhead of a function call if optimizations aren't enabled.
So, in this case, it's effectively a no-op. It's there, because the code that I adapted this from (the code to generate tzDatabaseNameToWindowsTZName and windowsTZNameToTZDatabsaeName) used byLine and thus needed to convert from char[] to string, whereas this code uses splitLines and thus is already using string. I didn't notice that the to!string was now unnecessary and should have removed it.
I'll remove the unnecessary to!string shortly.
|
So, is anyone going to merge this as-is, or do I need to figure out how to rewrite this to use std.xml, much as I'd prefer to avoid that? Or are there any other issues that anyone has with this PR as it stands? |
| } | ||
|
|
||
| /++ ditto +/ | ||
| TZConversions parseTZConversions()(string windowsZonesXMLText) @safe pure |
There was a problem hiding this comment.
Why a template? We have to put something in Phobos...
There was a problem hiding this comment.
To reduce what gets pulled in, particularly since not many programs are likely to be using it. Given its current implementation, it probably doesn't buy much, but initially I was going to have it reading in the file, which would have meant pulling in std.stdio, which I wouldn't want to do normally, and if it gets changed to use std.xml (or std.data.xml or whatever comes along to replace it), then I'd definitely want to templatize it to avoid that unnecessary dependency. But as it stands, it's not buying much, and I should probably untemplatize it.
It's proven to be a maintenance problem to maintain the time zone conversions between the IANA TZ database names and the Windows time zone names in std.datetime. So, rather than compiling them in, this provides a way to get them at runtime (which will also make it so in the future, older releases can work properly on up-to-date Windows boxes, which is not the case currently with regards to time zone conversions). The current conversion functions can be deprecated after parseTZConversions has been out for a release (so that it's possible for users to compile their code for two releases in a row without getting deprecation messages).
|
And yet again we needed to update the time zone conversions... #3945 It would be nice if we could get this merged and move towards being able to remove the hard-coded time zone conversions and the maintenance problems that they cause. |
|
Auto-merge toggled on |
Fix for issue# 15376. Get time zone conversions at runtime.
|
Thanks! |
It's proven to be a maintenance problem to maintain the time zone
conversions between the IANA TZ database names and the Windows time zone
names in std.datetime. So, rather than compiling them in, this provides
a way to get them at runtime (which will also make it so in the future,
older releases can work properly on up-to-date Windows boxes, which is
not the case currently with regards to time zone conversions). The
current conversion functions can be deprecated after parseTZConversions
has been out for a release (so that it's possible for users to compile
their code for two releases in a row without getting deprecation
messages).