zeke / remind-me

A command-line reminder tool that supports natural language dates and times, inspired by Slack's remind feature (UNMAINTAINED)
http://npm.im/remind-me
82 stars 11 forks source link

Updates to parse.py #63

Closed drodrig closed 7 years ago

drodrig commented 7 years ago

The updates herein provide three changes.

The first change sets result.time as a Date object when an interval is found by chrono (instead of a result.time value of seconds since epoch). The change helps unify the result.time object:

result.time = new Date(new Date().getTime() + result.interval)

The second change allows for a single-digit time within a reminder string. Currently chrono throws an exception. By adding the ":00" chrono can understand the time string (e.g. "remind me to go for a walk at 9"):

      if (typeof result.time === 'string') {
        if (result.time.length == 1) {
          // chrono barfs without a full time string
          result.time = result.time + ':00'
        }
        result.time = chrono.parse(result.time)[0].start.date()
      }   

The final change checks if chrono returned a time in the past and tries to futurize it by adding 12 hours, as chrono will sometimes return a time in the past:

      if (result.time < new Date()) {
        // Chrono can return a time in the past when information is limited.
        // Try adding 12 hours and re-parsing.
        result.time = new Date(result.time.getTime() + 12 * 60 * 1000) // add 12 hours
      }   
zeke commented 7 years ago

Awesome. Can you try adding tests for this new behavior?

drodrig commented 7 years ago

Some of your tests already address the reminders in the past situation. Running the tests actually revealed some bugs in my code, so I re-factored.

I modified one existing test (remind me {weekday} to {task}) to account for every day name of the week.

I added one test (remind me at {timeWithHourOnly} to {task}) to account for a single-digit number for the time of day.

I moved my code in lib/parse.py to its own function: futurizeReminder() for easier maintenance.

Question: Do I create a new pull request for the test file (test/parse.py)? Sorry, I'm new to working with github.

-------- Original Message -------- Subject: Re: [zeke/remind-me] Updates to parse.py (#63) Local Time: December 2, 2016 12:03 AM UTC Time: December 2, 2016 5:03 AM From: notifications@github.com To: zeke/remind-me remind-me@noreply.github.com drodrig drodrig@magicbrain.com, Author author@noreply.github.com

Awesome. Can you try adding tests for this new behavior?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/zeke/remind-me/pull/63#issuecomment-264374083, or mute the thread https://github.com/notifications/unsubscribe-auth/ACSc1FzyAKyyd6z1ENYfq8CjQUqLJJayks5rD6aOgaJpZM4LAsXS .

drodrig commented 7 years ago

I think we're good to go unless you need anything else. I see that the test files got added to the auto-magically when I committed them to my fork.

Running "npm test" gives me:

26 passing (121ms) 1 pending

zeke commented 7 years ago

Thanks! This change is published in 1.6.1

npm i -g remind-me@latest