Closed davispuh closed 7 years ago
@leejarvis have you taken a look on this? What do you think?
I think this looks good so far. There are a few methods in date_object
and time_object
and some other places that are a bit unwieldy so I'd be keen to clean those up but it looks like you've spent a lot of time on this. Great work!
I'll first finish this so that all tests pass and only then clean it up. Also for some places I'm not sure about best implementation so it would be nice if you can give any tips what could be made better.
Finally, I've something to show :)
So basically, I started this refactoring like a year ago or so. It was meant to be just few fixes, but that wasn't really possible as had to change a lot of things and now, it looks like it's quite a big rewrite. Still not yet fully finished and there's probably some bugs and things left to do but I wanted to show this what I've. All tests in
test_parsing
does pass, but some needed to be updated because previously Chronic handled incorrectly some cases. Other tests still need to be rewritten for new implementation.So anyway, what does this rewrite gives to us? This implementation will correctly parse all previously supported date/time formats with timezone support, which will resolve dozens of issues. Also DST changes won't ruin dates on other days (loads of issues like #179). And will give good foundation to implement a lot more date/time formats very simply. But most importantly Chronic will finally parse dates sanely. I'll try to explain, what I mean by sane parsing.
Take a look at this image and example code
Next really good part is, awesome tagging support (
definition.rb
), basically, it's a RegExp for date parsing, it should be really easy to add new date/time formats and override/monkey-patch if defaults doesn't suit. For examplethis definition will match only if all criteria will pass, that is, first token, must have
MonthName
tag (eg. jan, january, etc), then optionally eitherSeparatorSpace
(`) or
SeparatorDash(
-), then
ScalarDayand next it should not be
SeparatorSpacefollowed by
Unit(eg. 'year'), if it matches then
handle_mn_sdwill be called. In simpler words
:optionalmeans match ANY from array and
:none` means don't match this array.Okay, I think I've described main changes and I suggest to just look at code and try it yourself, some old Chronic issues should be already resolved here, but mostly this is just main work done to be much easier fix everything later and create the most robust parsing library :) I really like some parts of this implementation, but now since I had to add a lot of features can see some things are getting really complex and ugly. But still IMO this is way better than was before and most importantly can easily add all date/time formats you've ever wanted. This is just start and like I said it's not ready, so shouldn't be merged yet.