remind: add support for dates in future

[alanhuang122]

This pull request addresses issue #1148 by adding support for future dates, not just the next occurrence of the specified time.

[alanhuang122]

I'm unsatisfied with the code quality upon review. If this functionality is desired, I can make a new PR.

[dgw]

@alanhuang122 BTW, this functionality is desired. See #1148, which I've left open.

[alanhuang122]

I'll work on this RSN.

[dgw]

@alanhuang122 Same question as I just asked in the other thread: Do you still intend to finish this, or should someone else adopt this PR?

[alanhuang122]

Pushed a cleaned up version of the code, and force-pushed to appease Travis. Twice.

[dgw]

and force-pushed to appease Travis. Twice.

Story of my life 😹

I'll look at this & give it a good shakedown over the weekend, hopefully. Thanks for reviving it!

[dgw]

Request: Rebase to put 6169b82 as fixup into 99c1278, or even squash both into 4153fa1 (but mostly I just want to be rid of the commit labeled copyediting 😁).

More importantly, I was hoping this would be all ready to merge now that I got a minute to review it, but… there's some timezone nonsense going on:

<dgw>   .at 12:00 2019-01-01 test reminder with date/time
<Sopel> dgw: Okay, will remind at 2019-01-01 - 18:00:00UTC
<dgw>   .at 12:00CST 2019-01-01 test reminder with date/time
<Sopel> dgw: Okay, will remind at 2019-01-01 - 18:00:00UTC
<dgw>   .at 12:00:00CST 2019-01-01 test reminder with date/time
<Sopel> dgw: Okay, will remind at 2019-01-01 - 18:00:00UTC
<dgw>   .at 12:34:00EST 2019-01-01 test reminder with date/time
<Sopel> dgw: Okay, will remind at 2019-01-01 - 13:34:00EST
<dgw>   .at 14:00EST 2019-01-01 test reminder with date/time
<Sopel> dgw: Okay, will remind at 2019-01-01 - 15:00:00EST
<dgw>   .at 14:00PST 2019-01-01 test reminder with date/time
<Sopel> dgw: Okay, will remind at 2019-01-01 - 20:00:00UTC
<dgw>   .at 14:00America/New_York 2019-01-01 test reminder with date/time
<Sopel> dgw: Okay, will remind at 2019-01-01 - 15:00:00EST

Sopel's default_timezone is UTC, but the system I'm testing on has its clock set to America/Chicago—though it is an Ubuntu system, so the internal clock should be UTC anyway, AFAIK.

When the displayed time is using UTC, it's correct. CST is UTC-0600, EST is UTC-0500, and PST is UTC-0800. Not sure what's up with the time-shifting though. Like, 14:00America/New_York shouldn't be translated to 15:00:00EST—it's 14:00:00EST.

It being past 3am, I'm too tired now to continue digging. But I'm hoping that, as the author of this patch, you'll have quicker insight than I, anyway, @alanhuang122 😉

[alanhuang122]

Ack'd.
I do recall also finding some issue with timezones (probably the same) a few days ago, but haven't yet gotten around to porting the fix I (tried to?) make.

[alanhuang122]

Fixed and squashed (latest commit is preserved, for review).

[Exirel]

It would be much easier to review this with confidence if it has unit-tests. For that, I suggest to extract everything until duration = timediff.seconds + 86400 * timediff.days into its own function. Something like:

def get_duration(trigger):
    regex = re.compile(...)
    # ... hard work ...
    return timediff.seconds + 86400 * timediff.days

And then:

def at(bot, trigger):
    # ... 

    try:
        duration = get_duration(trigger)
    except:
        bot.say('Error...')
        return

    if duration < 0:
        duration += 86400
        duration += 86400
    create_reminder(bot, trigger, duration, message, timezone)
    if timezone:
        create_reminder(bot, trigger, duration, message, timezone)
    else:
        create_reminder(bot, trigger, duration, message, 'UTC')

Of course, if you don't have time or interest in working on this PR again, I'll take for myself, so we don't lost all this hard work.

[Exirel]

I did the thing in #1590, closing here. Feel free to comment/review the new PR, help is still and always be welcome.