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

Add single-character en time units #366

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

Conversation

Mitchell3514
Copy link

The title says it all.

@benaubin
Copy link
Collaborator

Hi there and Merry Christmas! This change looks great, but it could cause false positives. Would you be willing to refactor this into a configuration option so that it's opt-in?

@Mitchell3514
Copy link
Author

Hi, Merry Christmas also! I will take a look at that later.

And since French already parses 'h', want to do the same there?

@benaubin
Copy link
Collaborator

benaubin commented Dec 25, 2020

Sounds great! I'd prefer we keep existing library behavior as-is, as to not break dependent applications (so let's not change the french locale).

@Mitchell3514
Copy link
Author

Mitchell3514 commented Dec 25, 2020

The way it works now has not altered the existing behaviour as far as I can find. Test results are also the same as before (1 failed).

Before I start adding tests, I would like to know whether this is how you intended it to work?
One possible flaw is that this option is currently ignored in strict mode for the parsers that handled that option already.


Besides this, I also noticed it doesn't consistently* parse conjunctions, would that require a new Parser?
For example: parsing in 2 days and 5 hours results in 2 days. Whilst in 2 hours and 19 minutes parses 2 hours and 19 minutes.

@wanasit
Copy link
Owner

wanasit commented Jan 15, 2021

Hello sorry for my slow reply. Overall, really good works!

I know this is opposite to @benaubin suggestion, but I'm actually thinking we could detect those single character behavior by default. If we used those patterns in relative parsing, e.g. "in 25m" example, I think it almost always mean the time. (@benaubin do you have any significant false positive examples that you have in mind?)

One possible flaw is that this option is currently ignored in strict mode for the parsers that handled that option already.

Yes. strict mode is very subjective. And that's also the reason I think for the default (or casual) option, we should also detect those abbreviation by default.

If we really want to introduce another options, I'm thinking the useShorts should be true by default. And maybe includeAbbreviations might be more precise configuration name.


Besides this, I also noticed it doesn't consistently* parse conjunctions, would that require a new Parser?
For example: parsing in 2 days and 5 hours results in 2 days. Whilst in 2 hours and 19 minutes parses 2 hours and 19 minutes.

Yes. Let fix that separately.

@Mitchell3514
Copy link
Author

Mitchell3514 commented Jan 19, 2021

I don't have a preference for any of the cases (with/without option & on/off by default), as for our use case anything would work.

Whether it would be a problem to enable abbreviations by default, I'm not sure. In an edge case, 'In 25m' could mean in 25 meters, like when talking about route planning (which isn't particularly a use case for Chrono). It'll depend on how missing out on 'in 25m' as time versus a misinterpretation of 'in 25m' as distance weighs up to each other.

And yeah, I didn't have a good name for it. If the option makes it, then includeAbbreviations sounds good.

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