Watson-Personal-Assistant / SkillBoilerplate

Other
7 stars 63 forks source link

concat array of responses #11

Closed offerakrabi closed 6 years ago

offerakrabi commented 6 years ago

When receiving a response from wcs which contains more than one responses instead of returning only one of the responses we now return all of them concated.

erezbi commented 6 years ago

This change, currently, means that we lose support for 'random' from regex based skills. I am not sure we can do that.

troyibm commented 6 years ago

The original issue was that the BP was returning no response when one of the responses was empty string. That's the real issue we need to fix. WCS would return both the empty string and the non-empty string. Since the BP only returned one (and was always random), it would return empty string. My solution was to check the return string, if empty, pick the other (if there were two) and if more than than, then do random again and check.

erezbi commented 6 years ago

@troyibm I agree with @offerakrabi 's fix. We should not remove empty strings. Also, we should not chose only one response if the original WCS behavior is to return multiple responses. If the conversation in WCS is modeled using a few nodes for creating the full response, we should not break this behavior. We will concatenate the responses. This will not always solve your specific issues but will also support the full behavior of WCS. The 'random' support should not have been implemented there at all, and will be moved to the regex module when we get to it (hopefully next release)

Concatenating the responses should solve the empty strings issue too, as the response that the user will hear/read will be the non empty strings.

erezbi commented 6 years ago

@troyibm - we saw that in WCS you can have part of the response on one node, and the other part of the response on a child node in the tree, and if there is a jump between the first and the second that skips user input, then the user will see both responses.

imagine a node A with response 'Hi there,' with 3 child nodes with the condition being a context var named 'partOfDay':

The user is supposed to get either 'Hi there, Good morning' or 'Hi there, Good afternoon', or ... Offer's current fix still supports this behavior. 'Random' will break it. NOTE that 'Random' between multiple optional responses is already implemented in WCS. i.e. a user can specify in WCS multiple responses and chose either 'random' or 'in order'.

offerakrabi commented 6 years ago

@erezbi I changed the default behavior to just return the text. Note that this will happen only if the developer calls response.say and the 'selection' parameter is not 'all' or 'random'.

erezbi commented 6 years ago

@offerakrabi wouldn't this mean that we will return an array instead of a string? The default behavior was to return the first string in the response array, why shouldn't we keep that?