Made DateFormat Extensible #12799
Made DateFormat Extensible #12799
Conversation
New methodology works similiarly to how "promote_rule" works for promote.
|
Nice! cc: @quinnj |
| slotrule(::Type{Val{'M'}}) = Minute | ||
| slotrule(::Type{Val{'S'}}) = Second | ||
| slotrule(::Type{Val{'s'}}) = Millisecond | ||
| slotrule{c}(::Type{Val{c}}) = Union{} |
There was a problem hiding this comment.
rather than dynamically dispatching on Val{::Char}, i think this would be better represented as a Dict{Char, DataType} (less boilerplate and indirection, plus more features like iteration and default get):
e.g:
slotrule['y'] = Year
slotrule['m'] = Month
...
There was a problem hiding this comment.
You could also implement it with an array indexed by character offset:
const slotrule = Array{DataType}(53)
slotrule['y'-'e'] = Year
...There was a problem hiding this comment.
I originally had implemented this as a Dict but switched to using value types as made the code appear intended to be extensible rather than just happen to be. The one advantage of switching to a Dict gets us is that the regular expression in DateFormat can restricted to the letters used.
I'm intending to stick with the dynamic dispatch solution unless someone has a solid reason against it.
There was a problem hiding this comment.
I dislike the use of multiple dispatch to pun on a Dict (or any other primitive data structure for that matter). This is being used to form a simple Associative mapping, there is no need to hide that in the generic dispatch machinary. The use of Val is a strong indicator to me that this code is not optimal style. This is also much more verbose and less clear than using the actual data structure this is mimicking, it is substantially slower, you can't change or remove items, and extending it at runtime can cause you to run into code statepoints errors (I don't have great terminology for this error, but it's somewhat related to # 265).
There was a problem hiding this comment.
That's a much better argument for not using the value types. I'll make the switch to Dict.
There was a problem hiding this comment.
Why not use a simple lookup table? A Dict whose keys are bytes seems nuts. If you want it to be extensible to other ASCII characters you've already got the slots.
|
@StefanKarpinski: I may switch to using a lookup table yet. I mostly care about extensibility support so the implementation we end up using should be easy to understand. |
|
Why not just do it now? Skip subtracting an offset and just pre-allocate a 256-element array of types: const slotrule = Array{DataType}(256)
slotrule['y'] = Year
...Now it's extensible simply by assigning new types into the array. |
|
@StefanKarpinski You can't index with |
|
Right, still pretty simple to extend and likely to become more so. |
|
@StefanKarpinski: Things look good to you? Who should I mention to get this merged? |
|
Looks fine but we're in the 0.4 feature freeze window now, so it'll have to wait until after the release. |
|
Ok. That's kinda too bad. TimeZones.jl needed this to get rid of some code it needed to overwrite in Base.Dates. |
|
Yeah, this would really be nice to get in if only for proper TimeZone support. Where it's fairly isolated from the rest of Base code, it shouldn't disrupt anything else. Definitely understand though if we're a little too late here. |
|
Your call, @quinnj – the date/time code is your domain. I'm ok with it and this does look safe. |
|
@quinnj I'd be grateful if we could get this in for the 0.4 release. Let me know if you need me to do anything to make this happen. |
|
Thanks @omus. Just wanted to take some time to review. Looks good. Let the timezones zone! |
|
Sweet! Thanks for getting this done @quinnj! |
| @@ -183,6 +183,17 @@ end | |||
| @test Dates.lastdayofquarter(Dates.DateTime(2014,8,2)) == Dates.DateTime(2014,9,30) | |||
| @test Dates.lastdayofquarter(Dates.DateTime(2014,12,2)) == Dates.DateTime(2014,12,31) | |||
|
|
|||
| first = Dates.Date(2014,1,1) | |||
There was a problem hiding this comment.
this causes an imported binding for first overwritten in module TestDates warning, should really use a let block or a different variable name for these
There was a problem hiding this comment.
I'd just rename the variables from first and last to firstday and lastday.
When I was working on TimeZones.jl I found out that the DateFormat code was written in such a way that made it difficult to extend. As a temporary solution in TimeZones.jl I overwrote some of the
Base.Datesmethods such that I could include support for new slot letters 'Z' and 'z'.The following pull request mades the
Base.Datesimplementation ofDateFormateasier for the TimeZones.jl package to extend and additionally fixes some minor bugs.