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

Enhancement support times with periods/dots/fullstops, e.g. 7.15pm #35

Open
clach04 opened this issue Aug 13, 2021 · 1 comment
Open

Comments

@clach04
Copy link

clach04 commented Aug 13, 2021

Whilst I don't personally use times of the form 7.15pm (with a . rather than :), I have come across these in the wild and the people using this form thought it was standard.

I have a change and a test that adds support, but it fails 2 date tests:

          (function() {
            var start = getNow();

            if (start.getHours() >= 19)
              start.setDate(start.getDate() + 1);
            start.setHours(19, 15, 0, 0);
/*
diff --git a/sherlock.js b/sherlock.js
index f6265ef..1323e06 100644
--- a/sherlock.js
+++ b/sherlock.js
@@ -33,9 +33,9 @@ var Sherlock = (function() {
     inMilliTime: /\b(\d+) ?(s(?:ec(?:ond)?)?|ms|millisecond)s? ?(ago|old)?\b/,
     midtime: /(?:@ ?)?\b(?:at )?(dawn|morn(?:ing)?|noon|afternoon|evening|night|midnight)\b/,
     // 23:50, 0700, 1900
-    internationalTime: /\b(?:(0[0-9]|1[3-9]|2[0-3]):?([0-5]\d))\b/,
+    internationalTime: /\b(?:(0[0-9]|1[3-9]|2[0-3])[:\.]?([0-5]\d))\b/,
     // 5, 12pm, 5:00, 5:00pm, at 5pm, @3a
-    explicitTime: /(?:@ ?)?\b(?:at |from )?(1[0-2]|[0-2]?[1-9])(?::?([0-5]\d))? ?([ap]\.?m?\.?)?(?:o'clock)?\b/,
+    explicitTime: /(?:@ ?)?\b(?:at |from )?(1[0-2]|[0-2]?[1-9])(?:[:\.]?([0-5]\d))? ?([ap]\.?m?\.?)?(?:o'clock)?\b/,

// NOTE this breaks date tests; "12.03" and "12.12"
*/
            return test("Let's do the consult at 7.15pm this evening then.", "let's do the consult this evening then.", start, null, false);
          })(),

Is this something you would consider supporting? I'm not sure what the heuristic would be but perhaps if there is am/pm present treat it as a time otherwise treat it as a date as it does today?

(EDITED for formatting)

@clach04 clach04 changed the title Enhancement support times with periods/dots, e.g. 7.15pm Enhancement support times with periods/dots/fullstops, e.g. 7.15pm Aug 13, 2021
@neilgupta
Copy link
Owner

Hmm interesting. I've never seen anyone write 7.15pm but I can believe that people do. I'm open to supporting it if you can fix the root cause for the failing tests. Treating it as a date if no am/pm is provided sounds like a reasonable approach.

Off the top of my head, you could either accomplish that by adding a lookahead for am/pm to the period pattern, or add a hack in the matcher logic to prefer date over time if the match length is equivalent and the divider is a .. The latter feels hackier so I'd probably prefer the lookahead, but not sure yet if that will add other problems. Looking forward to the 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

No branches or pull requests

2 participants