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

Control how numbers on their own are treated #205

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andygeers
Copy link

Hi there,

This pull request includes one bug fix and one 'feature':

  1. If you parse the string "1st" (just an ordinal) then it was previously ignoring the "now" option (which is kind of the whole reason I would want to use Chronic) - I've now fixed this
  2. If you parse the string "1" (just an integer) then at present it always treats that as a time (1pm) - but my use case doesn't care about times, only ever dates. I've added an option "ambiguous_number_priority" which you can set to either :time (the default, current behaviour) or :date (which would treat "1" identically to the ordinal "1st")

if options[:ambiguous_number_priority] == :date
@@definitions[:date] << Handler.new([:scalar_day], :handle_sd)
else
@@definitions[:date] = @@definitions[:date].reject { |l| l.pattern == [:scalar_day] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this 214 line does nothing, there's no such pattern with just [:scalar_day]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this same commit adds it if : ambiguous_number_priority is :date. I was finding that these definitions seemed to persist between calls, so if you called it once with : ambiguous_number_priority => :date and then later with : ambiguous_number_priority => :time then it would break

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, it caches. Actually I think it's not good idea to change @@definitions, it's not thread safe and better would be just parse that option in some handler or something like that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe have a handle_lonely_int that decides for itself whether to treat it as a day or an hour based upon options?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that would work I think.

@davispuh
Copy link
Collaborator

I've been working to fix lot of things in Chronic, but it's taking more time than I thought it will and I still haven't finished yet. There will be Chronic::parse, Chronic::parse_time and Chronic::parse_date that would solve this issue with :ambiguous_number_priority, but won't be soon...

@davispuh
Copy link
Collaborator

I've also fixed issue with :guess allowing it to be more flexible as most people wouldn't think that guess means middle of span, but would rather want 00:00. I'll look if I can cherry-pick some my changes without breaking tests and send PR

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.

2 participants