probot / reminders

set reminders on Issues and Pull Requests
https://github.com/apps/reminders
ISC License
162 stars 24 forks source link

incorect date parsing: 31st Oct parsed as 1st Oct #34

Closed chris48s closed 5 years ago

chris48s commented 6 years ago

example: https://github.com/DemocracyClub/EveryElection/issues/360

screenshot at 2018-07-13 17 06 22

I'd expect this to say "@chris48s set a reminder for Oct 31st 2018".

Also, side issue: Is there a way to unset/reschedule this now that this has happened?

bkeepers commented 6 years ago

Thanks for the report. I just tried to create a test case for https://github.com/bkeepers/parse-reminder and it seems to parse your example fine.

I'll test it out here:

/remind me "check the chicken - is it done?" on 31st Oct at 10:30am

GitHub
bkeepers/parse-reminder
parse-reminder - a node module to parse natural language reminders into who, what, and when
reminders[bot] commented 6 years ago

@bkeepers set a reminder for Oct 31st 2018

bkeepers commented 6 years ago

@chris48s Very odd. I'm not able to duplicate it. I just pushed a test case to https://github.com/bkeepers/parse-reminder/compare/test-case-31st-oct and the test is passing.

If you can come up with a test case that reliably fails, I'd be happy to investigate further.

Also, side issue: Is there a way to unset/reschedule this now that this has happened?

2 options:

  1. Just set a new reminder, which will clear the old one.
  2. Remove the reminder label
GitHub
bkeepers/parse-reminder
parse-reminder - a node module to parse natural language reminders into who, what, and when
chris48s commented 6 years ago

I see. In the case I've posted, it looks like the fact that the reminder text contains the string "Oct" confuses things. If I change that test case to:

'remind me "check Oct BoundaryLine release - does it have GSS codes for CEDs?" on 31st Oct at 10:30am': {
  who: 'me', when: new Date(2017, 9, 31, 10, 30, 0, 0), what: '"check Oct BoundaryLine release - does it have GSS codes for CEDs?"'
}

.. it becomes easy to see what has happened:

{
-  "what": "\"check  BoundaryLine release - does it have GSS codes for CEDs?\""
-  "when": [Date: 2017-10-01T09:00:00.000Z]
+  "what": "\"check Oct BoundaryLine release - does it have GSS codes for CEDs?\""
+  "when": [Date: 2017-10-31T10:30:00.000Z]
   "who": "me"
}

I accept this is an edge case because the string is somewhat ambiguous, but it is possible to construct some more unexpected examples. For example the following test case:

'remind me test prod again after next deploy - this may be fixed on 31st Oct at 10:30am': {
  who: 'me', when: new Date(2017, 9, 31, 10, 30, 0, 0), what: 'test prod again after next deploy - this may be fixed'
}

parses the word "may" as the date, rather than the string "31st Oct", which I think would be unexpected behaviour for most users:

{
-  "what": "test prod again after next deploy - this  be fixed"
-  "when": [Date: 2017-05-01T09:00:00.000Z]
+  "what": "test prod again after next deploy - this may be fixed"
+  "when": [Date: 2017-10-31T10:30:00.000Z]
   "who": "me"
}

This is definitely a much more subtle bug than I first thought I was reporting, but it could be a useful improvement to try and address it. Perhaps if the input string contains >1 strings which could potentially be parsed as a timestamp, the one which describes the most specific timestamp should be used, rather than the first one in the string?

Another possible weighting factor could be proximity to the word "at" or "on". If there's >1 strings which could be parsed as a date and one of them comes after the word "on", that's probably the one we want.

stale[bot] commented 6 years ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

reminders[bot] commented 6 years ago

:wave: @bkeepers, "check the chicken - is it done?"