Merge lp://qastaging/~anarcat/ibid/remind-648463 into lp://qastaging/~ibid-core/ibid/old-trunk-1.6

8 +<<<<<<< TREE
9 * Kevin Woodland
10 +=======
11 + * Antoine Beaupré
12 +>>>>>>> MERGE-SOURCE

Borked merge

What Marco means is that since you branched from trunk, someone else has touched AUTHORS.

So you need to either:
bzr merge lp:ibid
Fix AUTHORS
bzr resolved AUTHORS
bzr commit -m "Merge from trunk"
bzr push

Or
bzr rebase lp:ibid
Fix AUTHORS
bzr resalved AUTHORS
bzr rebase-continue
bzr push

The first one puts the trunk changes after your changes (which is actually correct, but can get a bit messy). The rebase option puts the trunk changes *before* your changes (you need bzr-rewrite installed to do it), which messes with anyone who's already using your branch, but gives a prettier history.

- AUTHORS file has clashes in it.

- parser exceptions aren't handled

- the messages could do with a bit of punctuation and cleaning up

Otherwise, good start!

34 from ibid.utils import human_join, indefinite_article, identity_name, \
35 plural
36 +from ibid.utils import ago, format_date
37 +

Those should be joined.

For the addresponses, please use dictionary-style substitions, i.e.
112 + event.addresponse(u"okay, I will remind %s in %s", (who, ago(delta)))
becomes
event.addresponse(u"Okay, I will remind %(who)s in %(time)s", {
    'who': who,
    'time': ago(delta),
})

I fixed the clashes. Couldn't rebase as I don't have a recent enough bzr (!? debian squeeze?).

I feel that it's not a problem that parser exceptions are not handled: the bot just complains ("burp" and so on) and the user will figure it out... But I have implemented a handler anyways, I hope it's the right wording.

I have worked quite a lot on the messages (see bug report), so I would like specifics here before starting to hack at this again.

Thanks!

My comments in my last review still stand.

Yes, you should catch parser exceptions, otherwise user-mistakes show up in the bot-owner's logs. The current message isn't very good, many people don't know what parsing is. "Sorry, but I couldn't understand the time you gave me"

u'Generic timed reminders' <- that's a bit generic, something a little more descriptive would be good.

Nice to see a new name in my review list!

64 + event.addresponse(u'%s: %s asked me to remind you %s, %s ago.', (who, from_who, what, ago(datetime.now()-from_when)))

(and two lines down)

We generally don't include the "%s:" in private messages, so you might want to check event.public.

Your total_seconds variable appears to contain a number of microseconds, so it should be called total_microseconds.

68 + @match(r'^(?:remind|ping)\s+(?:(me|\w+)\s+)?(at|on|in)\s+(.*?)(?:(about|of|to) (.*))?$')

You're inconsistent between '\s+' and ' '. Use ' ' (match turns it into '\s+' automatically). You also don't need the ^ and $.

Also, the only thing you do with the variable how is add it to what -- why not just match use '(.*)?' instead of '(?:(about|of|to) (.*))?'. This will also let one say "remind Taejo in 10 seconds to feed the cat".

It would be nice if we could allow "remind Taejo to feed the cat in 10 seconds", which is more natural, but I realise that the regex might get a little tricky.

Please use full sentences in docstrings. Start sentences with capitals and end them with periods.

When I tried "ping me at 10:50", I got this:

Traceback (most recent call last):
  File "/home/max/devel/ibid/remind-648463/ibid/core.py", line 33, in _process
    processor.process(event)
  File "/home/max/devel/ibid/remind-648463/ibid/plugins/__init__.py", line 145, in process
    method(event, *match.groups())
  File "/home/max/devel/ibid/remind-648463/ibid/plugins/fun.py", line 182, in remind
    ibid.dispatcher.call_later(total_seconds, self.announce, event, who, what, from_who, now)
UnboundLocalError: local variable 'now' referenced before assignment

> Nice to see a new name in my review list!

Thanks for all the reviews...

> 64 + event.addresponse(u'%s: %s asked me to remind you %s, %s
> ago.', (who, from_who, what, ago(datetime.now()-from_when)))
>
> (and two lines down)
>
> We generally don't include the "%s:" in private messages, so you might want to
> check event.public.

Fixed.

> Your total_seconds variable appears to contain a number of microseconds, so it
> should be called total_microseconds.

That is incorrect: it's a number of seconds. It's a reimplementation of delta.total_seconds(). Notice how the microseconds get divided by 10**6 at the end of that line.

> 68 +
> @match(r'^(?:remind|ping)\s+(?:(me|\w+)\s+)?(at|on|in)\s+(.*?)(?:(about|of|to)
> (.*))?$')
>
> You're inconsistent between '\s+' and ' '. Use ' ' (match turns it into '\s+'
> automatically). You also don't need the ^ and $.

fixed. i also added please.

> Also, the only thing you do with the variable how is add it to what -- why not
> just match use '(.*)?' instead of '(?:(about|of|to) (.*))?'. This will also
> let one say "remind Taejo in 10 seconds to feed the cat".

the problem is we need a token here to avoid eating the whole "what" with the time regex. since the time string can be very flexible, i'm not sure i can match it with a regex, so I prefer to have a bunch of keywords that allows us to parse the string better.

I don't see why we can't do what you want here anyways:

09:33:21 <anarcat> ibicat: remind me in 10 seconds to feed the cat
09:33:21 <ibicat> anarcat: okay, I will remind you in 10 seconds
09:33:31 <ibicat> anarcat: you asked me to remind you to feed the cat, 10 seconds ago.

> It would be nice if we could allow "remind Taejo to feed the cat in 10
> seconds", which is more natural, but I realise that the regex might get a
> little tricky.

Yeah, and then the argument order in the callback would be screwed up. This is actually how I started too, but then I switched, I don't remember why.

Maybe the arguments could be "named"? I'm not sure how to do this in this context...

> Please use full sentences in docstrings. Start sentences with capitals and end
> them with periods.

Fixed.

> When I tried "ping me at 10:50", I got this:

Fixed.

Now onto other reviews.

> My comments in my last review still stand.
>
> Yes, you should catch parser exceptions, otherwise user-mistakes show up in
> the bot-owner's logs. The current message isn't very good, many people don't
> know what parsing is. "Sorry, but I couldn't understand the time you gave me"
>
> u'Generic timed reminders' <- that's a bit generic, something a little more
> descriptive would be good.

Both fixed.

I have also fixed the addresponse() to use dictionnaries instead of printf-style interpolation.

I have yet to fix the order and flexibility of the regex. I feel this is not critical and can be fixed later. I have introduce a bunch of changes here and I'm hesitant to break this furthermore.

One last comment here: to allow both "ping me in 5 about foo" and "ping me about foo in 5", we would need to allow for named subgroups to be passed as named arguments to the match function.

This doesn't document this as being possible:

http://ibid.omnia.za.net/docs/trunk/api/ibid.plugins.html

... so I will not struggle with it for now. But basically, I think what I would like is this:

    @match(r'(?:please )?(?:remind|ping) (?:(^P<who>me|\w+) )?(?P<at>at|on|in) (?P<when>.*?)(?:(?P<how>about|of|to) (?P<what>.*))?')
    def remind(self, event, who, at, when, how, what):

... and of course whatever is responsible for calling remind() would need to pass the regex dictionnary. This would also require to have multiple regex in the @match() block to allow us to have different patterns that would match the same callback (is that possible now?).

tumbleweed mentionned on irc how named groups would be useful for i18n eventually.

oh, and I have more stuff to push, the dictionnary thing is broken. hold on.

<Taejo> Ibido: remind me at 11:00 about the cat
<Ibido> Taejo: I can't travel in time back to 22 minutes and 33 seconds ago (yet) so I'll tell you now about the cat

Haha, very nice.

Aaaaalmost there.

> from datetime import datetime

That belongs with the stdlib imports

16:54 <@tumbleweed> tibid: remind me in half an hour about DL meeting
16:54 < tibid> tumbleweed: Sorry, I couldn't understand the time you gave me
16:54 <@tumbleweed> tibid: remind me in 30 mins about DL meeting
16:54 < tibid> tumbleweed: Sorry, I couldn't understand the time you gave me
16:54 <@tumbleweed> tibid: remind me in 30 min about DL meeting
16:54 < tibid> tumbleweed: Sorry, I couldn't understand the time you gave me
16:54 <@tumbleweed> tibid: remind me in 30 minutes about DL meeting
16:54 < tibid> tumbleweed: okay, I will remind you in 30 minutes

Pity abbreviations don't work...

> "okay, I will"...

That should be "Okay, I will"...

> "you asked me"

Same there

135 + if what:
136 + event.addresponse(u"okay, I will remind %(who)s in %(time)s", {
137 + 'who': who,
138 + 'time': ago(delta)
139 + })
140 + else:
141 + event.addresponse(u"okay, I will ping %(who)s in %(time)s", {
142 + 'who': who,
143 + 'time': ago(delta)
144 + })

Seems like unnecessary duplication. Easily solved with %(action)s.

Whoops:

2010-10-07 16:59:43,336 ERROR core.dispatcher: Exception occured in Remind processor of fun plugin.
Event: {'account': 1, 'responses': [{'reply': u"I can't travel in time back to 15 hours, 59 minutes and 43 seconds ago (yet) so I'll tell you now to test", 'address': True, 'target': u'tumbleweed!<email address hidden>', 'conflate': True, 'source': u'atrum'}], 'source': u'atrum', 'addressed': True, 'processed': True, 'time': datetime.datetime(2010, 10, 7, 14, 59, 43, 330179), 'identity': 1, 'message': {'raw': u'remind me at 1am to test', 'deaddressed': u'remind me at 1am to test', 'clean': u'remind me at 1am to test', 'stripped': u'remind me at 1am to test'}, 'type': u'message', 'public': False, 'channel': u'tumbleweed!<email address hidden>', 'sender': {'nick': u'tumbleweed', 'connection': u'tumbleweed!<email address hidden>', 'id': u'tumbleweed'}}
Traceback (most recent call last):
  File "/home/stefanor/bzr/ibid/stefanor/tibid/ibid/core.py", line 29, in process
    processor.process(event)
  File "/home/stefanor/bzr/ibid/stefanor/tibid/ibid/plugins/__init__.py", line 145, in process
    method(event, *match.groups())
  File "/home/stefanor/bzr/ibid/stefanor/tibid/ibid/plugins/fun.py", line 201, in remind
    ibid.dispatcher.call_later(total_seconds, self.announce, event, who, what, from_who, now)
  File "/home/stefanor/bzr/ibid/stefanor/tibid/ibid/core.py", line 109, in call_later
    return reactor.callLater(delay, threads.deferToThread, self.delayed_call, callable, event, *args, **kw)
  File "/usr/lib/python2.6/dist-packages/twisted/internet/base.py", line 693, in callLater
    "%s is not greater than or equal to 0 seconds" % (_seconds,)
AssertionError: -57584 is not greater than or equal to 0 seconds

17:28 <marcog> remind me in 0 minutes about food
17:28 <tibid> okay, I will remind you in
17:28 <tibid> you asked me to remind you about food, 12 milliseconds ago.

How about adding an "announce <announcement> at 5pm"? Perhaps a second iteration.

17:29 <marcog> remind me to eat in 3 seconds
17:29 <tibid> Hello remind me to eat in 3 second

This ordering should work. The response is probably a weird match-alot wildcarded factoid, so ignore that.

I like "announce" without any target.

As anarcat noted, alternative orders will be much easier when we fix #656201 (Named groups in match regexes) -- and I'm planning to do that soon.

12:32 <Vhata> tibid: remind me at 12:30 about foo
12:32 <tibid> Vhata: I can't travel in time back to 2 minutes and 16 seconds ago (yet) so I'll tell you now about foo

It's cute, but that's not what a human would do. I'd prefer: "I assume you mean 12:30 tomorrow - I'll tell you about foo then" (or something)

13:26 < drubin> Maaz: remind drubin to follow up on
                http://wiki.ubuntu-women.org/Courses/IRC
13:26 < Maaz> drubin: Excuse me?
13:26 < drubin> bad bot

I know Maaz doesn't have this brach merged, but AFAIK we don't handle this case of "remind me about X" without a time. I can't think of a best way to handle it except for maybe sending a memo the first time the user does something after one hour.

> 17:29 <marcog> remind me to eat in 3 seconds
> 17:29 <tibid> Hello remind me to eat in 3 second
>
> This ordering should work. The response is probably a weird match-alot
> wildcarded factoid, so ignore that.

I don't see that here.

16:25:17 <anarcat> ibicat: remind me to eat in 3 seconds
16:25:18 <ibicat> anarcat: Sorry...

Furthermore, I think that problem is related to the fact we don't allow named regex match groups which would allow me to support variable orders like this. For now, I am limited by the API and can't match both:

remind me in 1 minute about foo

and

remind me about foo in 1 minute

so we'll have to decide on which for now.

> Aaaaalmost there.
>
> > from datetime import datetime
>
> That belongs with the stdlib imports

Not sure what this means, should I move the import? Should I "import as"?

> 16:54 <@tumbleweed> tibid: remind me in half an hour about DL meeting
> 16:54 < tibid> tumbleweed: Sorry, I couldn't understand the time you gave me
> 16:54 <@tumbleweed> tibid: remind me in 30 mins about DL meeting
> 16:54 < tibid> tumbleweed: Sorry, I couldn't understand the time you gave me
> 16:54 <@tumbleweed> tibid: remind me in 30 min about DL meeting
> 16:54 < tibid> tumbleweed: Sorry, I couldn't understand the time you gave me
> 16:54 <@tumbleweed> tibid: remind me in 30 minutes about DL meeting
> 16:54 < tibid> tumbleweed: okay, I will remind you in 30 minutes
>
> Pity abbreviations don't work...

Yeah, sorry..

> > "okay, I will"...
>
> That should be "Okay, I will"...

fixed.

> > "you asked me"
>
> Same there

fixed.

> 135 + if what:
> 136 + event.addresponse(u"okay, I will remind %(who)s in
> %(time)s", {
> 137 + 'who': who,
> 138 + 'time': ago(delta)
> 139 + })
> 140 + else:
> 141 + event.addresponse(u"okay, I will ping %(who)s in
> %(time)s", {
> 142 + 'who': who,
> 143 + 'time': ago(delta)
> 144 + })
>
> Seems like unnecessary duplication. Easily solved with %(action)s.

fixed.

> Whoops:
>
[...]
> AssertionError: -57584 is not greater than or equal to 0 seconds

also fixed.

> 17:28 <marcog> remind me in 0 minutes about food
> 17:28 <tibid> okay, I will remind you in
> 17:28 <tibid> you asked me to remind you about food, 12 milliseconds ago.

This is a bug with "ago()". I have now a special handler in my code for this, but I consider this bug outside the scope of this...

I have added "remind" the list of allowed aliases for the "tell" plugin so that you can do:

17:15:10 <anarcat> ibicat: remind anarcat about foo
17:15:11 <ibicat> anarcat: Yessir
17:15:13 <anarcat> foo
17:15:14 <ibicat> anarcat: By the way, anarcat on IMC told me "remind anarcat about foo" 3 seconds ago

Now we could expand the regex of the tell, and ideally merge those two plugins, but I think this is currently okay and refactoring could happen after merge... don't you think? :)

Of course I approve my own changes here. :) I think I have repeatedly addressed all concerns addressed here.

The *only* things that would remain here, but I think should be in a second phase, are:

 1. "announcements" for everyone to hear
 2. merge with the memo plugin: share the syntax and the decorators
 3. merge with memo: make a memo if the person is not there at the given time

Since i do not think those should be blocking the merge, i am approving this. :)

> Not sure what this means, should I move the import? Should I "import as"?

I mean with the first block of imports, the ones from the standard library, as opposed to being next to dateutil. Above unicodedata, not above dateutil.

> Of course I approve my own changes here.

That's taken for-granted :P You shouldn't review your own merge request.

I see a conflict in memo. In fact, I don't understand why we change memo, now memo conflicts with remind:

22:45 < tumbleweed> tibid: remind me in 5 minutes about something
22:45 < tibid> tumbleweed: Got it, I'll remind me on atrum

That's presumably not what we want.