Closed stephencpope closed 10 years ago
I can't speak to the logic of the change, which @karbarcca will have to review, but you certainly nailed your first pull request. This is great.
Sorry for the slow response here. Yes, this is excellent. Thanks for the thorough writeup and tests. So you're aware, I'm merging this, but not actively developing the package as it's being migrated into base julie. Either way, I'll definitely make sure your work gets in.
So should I somehow connect up with the base julia process? I ask because after getting a good grade with this patch, I had two extensions for you. The first is trivial: adding a public wrapper to the _day2date() method, because this is very handy to do things like:
d = date(2014,1,16) ... dt = datetime(Datetime._day2date(d)..., 11, 41, 0, 0, MST)
which avoids redoing all the Rata Die math by using the year(), month(), and day() methods.
The second, which I consider more essential, is to extend the TimePeriod support to milliseconds. Right now there's no reasonable way to do DateTime math at millisecond precision. However, 32 bits isn't really enough for millisecond offsets (that's about 24 days max), so I've implemented it as:
bitstype 64 MilliSecond{C<:Calendar} <: TimePeriod
and provided all the hair to interoperate with DateTime and the other TimePeriod. Mostly, being 64 bit means that there have to be a few extra convert() overloads to remove ambiguities, and some of the method definitions you fold into a loop over the PeriodTypes need to be done outside those loops to make them Int64 instead of Int32.
I can provide all this today, but then I will be out of the office for a week starting tomorrow so I couldn't support (or argue for) my updates.
Definitely good thinking, in fact, I've already done all that work :) Check out the pull request that I'm actively working here: https://github.com/JuliaLang/julia/pull/5328/files. In particular, all Period types (including Millisecond
) are now 64-bit and defined as immutables instead of bitstypes (I can elaborate on the reasoning if you want, but for all intents and purposes, it doesn't change the interface much).
I like the idea of a public _day2date
method as well.
oh, and one other addition I forgot: datetime2unix(), which is just the inverse of unix2datetime():
datetime2unix{T<:Offsets}(dt::DateTime{CALENDAR,T}) = int64(dt) - leaps(int64(dt)) - UNIXEPOCH
So, I will perhaps leave you with that, and not submit any pull requests, but instead wait for it to appear in Julia base.
b.t.w. immutable is great, as is having Seconds, Minutes, and Hours be 64 bit so that they can actually match the full range of DateTime.
oh, and my public _day2date is called ymd() but I don't really care :-)
The Datetime package convert method for Date -> DateTime ignores leap seconds, leading to the following unexpected behavior:
The problem is the following conversion method:
After my proposed corrections:
At the same time, I added support for specifying a non-default timezone, so that a DateTime which is midnight in the specified zone on the specified date is created:
By the way, the issue was missed in test.jl because Date -> DateTime conversion was only tested for using date(1,1,1) which is prior to any leap seconds. I have also added a few appropriate tests at the end of test.jl, because I wasn’t sure exactly where else to best insert them.
Please excuse any improprieties on my part, I am new to Julia, GitHub, and the process of submitting pull requests. Kindly let me know if there's a better way to do it!