Execute object macros within conditionals

[kirsle]

This change fixes a regression caused by #78 where <call> tags inside conditionals weren't being fully executed, due to the actual processing of the <call> tag being moved into a separate function from processTags() and the condition checking code didn't call this new function.

@callmephilip can you look at this code change and verify that it makes sense? One thought I have about it is that asynchronous object macros shouldn't be callable in a condition, because RiveScript requires the result to be ready "now" for comparison checking (I might need to pass in an explicit false for the async parameter, regardless of the one the user requested).

This will fix #107.

[kirsle]

I'll also fix #102 with this PR (a simple regexp change to make the {weight} tag gobble up surrounding spaces).

[davidchriqui]

@kirsle I've not find any public method allowing to call a redirection tag {@redirection trigger} directly from a subroutine, in the source code. Did I miss something ?

This would be very useful as you seem to not want asynchronous object macros be callable in a condition, as a workaround guessing you do the conditional comparisons directly from the subroutine.

[kirsle]

@davidchriqui the {@redirection} tag essentially just calls reply() again with the redirected trigger (it actually calls the internal _getReply function so it can increment the recursion depth counter and skip the >begin block, but for most use cases you can just call reply() again yourself with the new trigger).

Implementation of the {@...} tag from src/brain.coffee:

    # Inline redirector
    match = reply.match(/\{@([^\}]*?)\}/)
    giveup = 0
    while match
      giveup++
      if giveup >= 50
        @warn "Infinite loop looking for redirect tag!"
        break

      target = match[1]
      @say "Inline redirection to: #{target}"
      subreply = @_getReply(user, target, "normal", step+1, scope)
      reply = reply.replace(new RegExp("\\{@" + utils.quotemeta(target) + "\\}", "i"), subreply)
      match = reply.match(/\{@([^\}]*?)\}/)

What you could do:

> object test javascript
    var redirect = rs.reply(rs.currentUser(), "the trigger you want to redirect to");
    return "Redirected reply: " + redirect;
< object

[davidchriqui]

What about using replyAsync instead of reply (using async subroutine) ? Can you give a working example please ?

[kirsle]

Are you doing this inside a condition? (Like, * <call>something</call> == a => result and the "something" needs to do a replyAsync)? Because that's getting into some kinda dicey territory...

The reason I think object macros in conditions shouldn't be asynchronous is that RiveScript wants to check the condition "now" to see if it's true. So what happened up to that point was: the user's message matched a trigger, RiveScript is trying to find a reply for them, the trigger has conditions to check, and it needs to evaluate each condition and see what its return value is. If the condition is asynchronous, that can oftentimes mean the object macro will take some indeterminate amount of time to finish (e.g. call out to a web API or something). If the object macro takes, say, 5 seconds to resolve its promise, then RiveScript would be essentially frozen solid for 5 seconds waiting to see how that object macro finishes, and if it doesn't even return a "true" answer it just wasted a lot of time for nothing.

Asynchronous object macros return a Promise object, and rs.replyAsync() also returns a Promise, so you might be able to just have your object macro do something like:

> object test javascript
    return rs.replyAsync(rs.currentUser(), "the trigger you want to redirect to");
< object

I don't know how that will behave in a conditional, though. It likely won't work, because the condition internally doesn't know how to follow/resolve promises. And if I wanted to make it try to resolve promises, I don't even know how I'd go about doing so, given that _getReply() is currently one giant monolithic function that does all its work synchronously, and trying to get a block of code to just "pause" and wait for a promise to resolve wouldn't be very easy.

[kirsle]

If you really need to do something asynchronously like this, it might be better to just keep things simpler and do 100% of your work inside the object macro.

+ what is the weather like
- <call>weather</call>

> object weather javascript
    return new rs.Promise(function(resolve, reject) {
        // do whatever API stuff, and manage conditons yourself
        if (weather === "sunny") { resolve("The weather is sunny") }
        else if (weather === "cloudy") { resolve("It's cloudy outside") }
    });
< object

So instead of doing stuff like, * <call>weather</call> == sunny => The weather is sunny. just have all that condition logic inside the object macro directly.

(On a side note, even if the conditions could work asynchronously, if you had something like the above where it calls some API and then gives multiple answers depending on the result... each answer would be its own separate *Condition and would need to invoke the weather macro once each for every test, so if you had 5 different weather conditions and each request took 5 seconds to resolve, RiveScript would be "frozen solid" for 25 seconds in the worst case because it'd need to invoke the function over and over again until one of the results is true).

[kirsle]

After more experimentation, supporting asynchronous object macros (that return promises) in the left side of a conditional line can't work.

I was hoping I'd be able to call @processCallTags(left, scope, async) and then immediately wait/resolve them before checking their result (basically forcing the asynchronous macro to finish before continuing), but this doesn't seem to be possible in JavaScript, at least not in an ES5 compatible way (ES6 has generator support where you can kinda finagle promises to resolve in a specific order by yielding one at a time, though this would still require some non-trivial refactoring of the code).

So, when the left side of the condition is evaluated each object macro is called with an explicit false value for the async parameter (even if you used replyAsync() originally). This will cause an async object to return the error text that you should use "replyAsync" and the condition isn't truthy and fails too.

I updated the documentation in the reply-async example to make it clear to anyone who stumbles upon it that they can only use synchronous macros (which return strings) in the left side of conditionals.

[kirsle]

This also fixes #123.