search

facebook / duckling

Language, engine, and tooling for expressing, testing, and evaluating composable language rules on input strings.
Other
4.05k stars 720 forks source link

Making intervals 'inclusive - inclusive' #649

Closed emlautarom1 closed 2 years ago

emlautarom1 commented 3 years ago

I know that there are several questions related to how intervals are handled inclusive - exclusive. So far, I've found the following issues:

Nevertheless, I was wondering what would I need to change in the code to get the behavior inclusive - inclusive. As far as I understand, the reason of the current behavior is the following function:

https://github.com/facebook/duckling/blob/84175d61d6f44d0db2abb42d67c6c4213762b64f/Duckling/Time/Types.hs#L834-L849

In particular, the line

https://github.com/facebook/duckling/blob/84175d61d6f44d0db2abb42d67c6c4213762b64f/Duckling/Time/Types.hs#L843

Instead, if we did Closed -> fromMaybe s2 e2 we would effectively get the desired behavior - inclusive - inclusive.

Now, is this correct? Is there any other part of the code that assumes that intervals are inclusive - exclusive or is this change enough to make things work? I've tested it locally and, besides the fact that the whole test suit fails with mismatched predicates, the only issue is that some languages like Chinese have ambiguous parses.

chessai commented 2 years ago

I think you are correct wrt the code, but changing this would break essentially every user of duckling, so I'm not really in favour of changing it.