On Sun, 25 Sep 2011, Zefram wrote:
This is a total rewrite of DateTime::TimeZone. The main purpose of the
change is that, as we've discussed for the past couple of years, it uses
DT:TZ:Olson (and thus the standard tzfiles) for Olson zones. It ought
to be a drop-in replacement for the existing DT:TZ. Please try it out,
to see if it actually succeeds as a replacement. I'd particularly like
to know whether the new DT:TZ:Local code behaves itself on Windows, VMS,
HP-UX, and AIX.
First, thanks for working on this. I think getting this shipped will be a
big improvement for DateTime users!
Now on to questions and criticisms ...
----
Why borg the Windows and HP-UX code into this distro instead of letting
other maintainers handle this? For Windows, David Pinkowitz has said that
he'd be willing to maintain a DT::TZ::Win32 distro, and we already have a
separate HPUX distro.
I think this makes more sense since these maintainers have access to the
relevant systems, and probably have more motivation to keep the code up to
date.
If you borg it, you are stuck handling bug reports for systems you don't
really know anything about.
As far as not automatically recognizing new OS-specific distros, that's
fine since a new distro is an incredibly rare event.
----
If you want to document DateTime::TimeZone as not a class, I think having
a documented "DateTime::TimeZone::new" function is really weird. It should
probably be named something else, like "load_zone" (I don't really care,
just not "new".
Obviously, the old DT::TZ->new behavior would need to work too, but it
doesn't need to be documented, and once this code is released, I can
adjust DateTime.pm to use the new APIs.
----
This module breaks the DateTime.pm test suite. The failures look to be
from some minor API differences. One of them was clearly a bug in the test
suite (expecting 0 for a boolean instead of any false value). I've fixed
that in HEAD, but the other failures seem to come from a change in an
exception's error message.
The exception that's thrown for an invalid local time is different with
this new version of the library. On one hand, I don't think this error is
documented. OTOH, I don't see a reason to break anyone's code by changing
this for no reason. Can you make the error the same in the new code?
That'd be ideal.
-----
The DateTime::TimeZone::Catalog module needs to exist, if only for
documentation purposes. I think it'd make sense to add a generated
Time::OlsonTZ(::Data)?::Catalog document to the Time-OlsonTZ-Data distro.
This could just be a .pod file. Then the new DT::TZ can ship a Catalog.pm
that just says "see Time::OlsonTZ for a list of all zones".
---
Are there are any 32/64 bit issues we need to deal with? With the existing
implementation, you can get time zone information for years after 2038,
even though the system-installed Olson database doesn't go that far into
the future. Yes, I know that information is basically meaningless, but I
think in this case it's better to not throw an exception.
What happens if a user on a 32 bit system with a 32 bit Perl installs the
Time::OlsonTZ::Data distro and tries to create a DateTime 100 years in the
future in the America/New_York zone? I'm guessing it just works, but I'd
like to know that for sure ;)
----
I think it also needs some docs on what's changed from an end user
perspective. The big one is that in order to get Olson updates, the user
now need to keep track of the Time::OlsonTZ::Data distro, not DT::TZ. That
need a big flashing warning both in Changes and the main docs. I'll also
make a big note of this when I release a DateTime.pm that depends on
this new distro.
The COMPAT document you attached is good, but it's missing this key point.
----
I'd really like to see this code in a public repo before it's released,
along with the various other modules this depends on. I'm fine with
github, or I could host it on my own git server (which mirrors everything
to github anyway). But if you put it on github yourself you'll own the
canonical repos.
As far as maintenance, I would like to have comaint, although I'm not sure
how much good that will really do me since all the real work is done in
other distros anyway. But it'll make me feel better ;)
Thanks again for working on this.
-dave
/*============================================================
http://VegGuide.org http://blog.urth.org
Your guide to all that's veg House Absolute(ly Pointless)
============================================================*/
