Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests for Chronic::Parser#pre_normalize #285

Closed
wants to merge 2 commits into from

Conversation

monicao
Copy link

@monicao monicao commented Nov 12, 2014

I added some tests to the beta branch to understand how the pre_normalize method works. They're quick and dirty tests, but they might be useful.

A couple of things stood out.

Chronic::Parser.new.pre_normalize('a quarter to six')
#=> '1 / 4 minutes past 6'

Chronic::Parser.new.pre_normalize('three quarters until the report comes in')
#=> '3 q until the report comes in'

Chronic::Parser.new.pre_normalize('three quarters till the report comes in')
#=> '3 / 4 minutes past the end of the year'

@davispuh
Copy link
Collaborator

Note that all of pre_normalize regexp's will be removed with #278 (not done yet) because they're just generally a hacks and proper implementation will actually parse that.

@monicao
Copy link
Author

monicao commented Nov 12, 2014

Thanks for the heads up. I am working porting this library to javascript.

How stable is the rewrite branch? It seems a lot cleaner. Do you think I should just port that branch instead?

@davispuh
Copy link
Collaborator

Well, IMO I think it would be better to write from scratch than port this, just take main idea from that rewrite branch. Tokenization, Taging, Objects, Token Groups, flexible Definitions those are generally good concepts. Only this exact implementation might not be the best, there are few things I don't really like 100% but haven't figured out any better way. As for stability, it currently works for most cases and all tests from test_parsing does pass, but not other tests. There probably won't be much big changes anymore, just generally some fixes there and here as I'm pretty sure there are some edge cases not handled. Also need better test coverage and then will see how good it is. Basically need a test for every date/time format combination to be sure.

By the way do you know about Opal? then wouldn't need to manually do anything ;)

@monicao
Copy link
Author

monicao commented Nov 14, 2014

Yeah, I'm not planning to port his library over blindly, but I will definitely 'borrow' the regexes and overall design. You guys already solved the hard problems.

I'd be happy to contribute some test for the different date/time combinations. Definitely a lot of edge cases there.

Opal looks really cool, but I'll write this by hand because the library will probably be a little bit more maintainable if a human writes it. :)

@leejarvis
Copy link
Collaborator

I've be wanting to rewrite Chronic from the ground up, and I've actually been working on it for a couple of years. Here's an old Gist of how it looks. However, this code still isn't ready for any kind of release, even if it improves in Chronic a lot. @davispuh has done great work on the Chronic rewrite which will hopefully cover most concerns anyway.

Porting something like Chronic to JavaScript is a mammoth task, so I take my hat of to you for even considering it. I think momentjs covers most use-cases, although I don't know of anything completely similar to Chronic in any other languages. It's a unique beast.

@monicao monicao closed this Nov 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants