Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

severe bug in [] optional matching #48

Closed
IvoJager opened this issue Oct 10, 2015 · 8 comments
Closed

severe bug in [] optional matching #48

IvoJager opened this issue Oct 10, 2015 · 8 comments

Comments

@IvoJager
Copy link

Hi,

Unless this expected behaviour, there seem to be a pretty bad bug in the way optionals are matched.

Given the following rule;

+ (aa|bb|cc) [bogus]
- TRIGGER  

an input of "aabogus" (notice no space!) gives an output of "TRIGGER", completely ignoring the space in the rule.
Contrast this with the use of an alternative;

+ (aa|bb|cc) (bogus)
- TRIGGER  

an input of "aabogus" (again, notice no space) does not match, respecting the space in the rule.

Worse, something like this

+ (yo|hi) [computer|bot] *
- TRIGGER 

Causes an input like "your fly is undone" "yoghurt is yummy" "hide and seek is fun" and "hip hip hurrah" to match.

In the off chance this is expected behaviour, I'd love to know what I'm doing wrong and how to properly construct my rules...

Tx!

@IvoJager IvoJager changed the title severe bug in in [] alternative matching severe bug in in [] optional matching Oct 11, 2015
@IvoJager IvoJager changed the title severe bug in in [] optional matching severe bug in [] optional matching Oct 11, 2015
@kirsle kirsle closed this as completed in 02f236e Oct 11, 2015
@kirsle
Copy link
Member

kirsle commented Oct 11, 2015

I confirmed this was an issue. It affects all implementations of RiveScript; I believe I've fixed it now for this JavaScript version. Can you try testing with the latest master branch?

The root cause of the issue was that the original regexp was surrounding each of the optionals with \s* (matching zero-or-more spaces), and to cover the case where the user didn't provide any match for the optional, it would use \s* by itself. This meant the user didn't require a space around the optional parts and it'd still match, and the user could type words that match opposite ends of the optional with no space in between and that would also match.

I changed it so it uses [\b\s]+ in place of all the \s* parts, so it requires there to be at least one space or word boundary in all cases. This still works with the current unit tests for the optionals feature and it fixes all of your test cases too (I added your test cases to the unit test file).

02f236e This is the change I made.

I'll make that change on the other implementations of RiveScript as well.

@IvoJager
Copy link
Author

Perfect! I don't have coffee/ruby installed on my box - I'm using the 1.1.4 'distro'/browserified version, but I patched that version with your changes and everything behaves as expected now.

Your explanation makes perfect sense. Thanks for the lighting fast response as well. (FYI I put a similar report on chatbots.org - feel free to delete that topic/thread).

@kirsle
Copy link
Member

kirsle commented Oct 11, 2015

I released version 1.1.6 that includes this fix. :)

@fritz-smh
Copy link

Hi,

I integrated rivescript 1.0.6 (python interpreter) in Domogik home automation system for our butler (basically sort of a jarvis for home automation).

I integrated the new 1.8.0 release today, and some of my triggers (a lot) didn't match anymore. I just find that this ticket is the root cause.

Here are some example :

An english one :

+ foo apple[s] bar
- TRIGGER

A french one :

+ quel[le] est la météo
- TRIGGER

The way I used [] was very usefull to put optionnal parts in some words (related to singulat and plural, masculine and feminine).
Could you confirm me than now the only way to do this is to put each word in its complete form for each way to write it ? For example :

+ (quel|quelle) est la météo
- TRIGGER

+ foo (apple|apples) bar

Thanks,
Fritz

@kirsle
Copy link
Member

kirsle commented Nov 20, 2015

Sorry this broke your code.

The intended way to handle singular vs. plural is as you said,

+ foo (apple|apples) bar
- TRIGGER

You could also use arrays if you're going to use the same words frequently, like

! array apples = apple apples

+ foo @apples bar
- TRIGGER

I hadn't thought of optionals or alternatives used in such a way that they touch other words in the trigger, so that wasn't a supported syntax. I also don't know how to support both versions; it was definitely a bug the way it was handled before, because spaces around an optional word could be omitted and cause matching to happen in an unintuitive way.


Also, the following should work now for plurals which may be a nicer syntax (as of RiveScript-JS v1.1.7 which I just released; a bug before was causing arrays not to work properly, at all probably...):

! array s = |s

+ foo apple@s bar
- Match

This makes an array named s which is either the letter s or nothing. So for simple plurals you can just put @s at the end of a word. Arrays can be expanded anywhere in a trigger, even when it's touching another word.

@fritz-smh
Copy link

Hi,

Thanks for your quick response!
The array idea is a good one for plural. Is it also possible to use it with () ?. Example :

+ (quel@s|quelle@s) est la meteo
- trigger

This way, I would avoid to do :

+ (quel|quelle|quels|quelles) est la meteo
- trigger

Thanks,
Fritz

@fritz-smh
Copy link

Hi again, a (maybe) last question : for my purpose, in french I would like to do an alias for some pronouns :
le
la
les
l'
...

if I use an array like this :

! array pronoun = le la les l'

To be able to use them with a space around, I will have to do :

// example : allume la lumiere, allume le salon (switch on the light, switch on the living room)
+ allume @pronoun *
- trigger

But for this example it will not be good :

allume l'etage  (switch on the first floor)

Because I think it will be triggered only If I put a space after the '. Like this :

allume l' etage

How would you handle this case ?

Thanks,
Fritz

@kirsle
Copy link
Member

kirsle commented Nov 20, 2015

I found that this works:

! array pronoun = le la les l'

+ allume @pronoun *
- triggger

+ allume @pronoun*
- trigger 2
You> allume l'etage
Bot> trigger 2
You> allume l' etage
Bot> triggger

Also this works too but looks a little uglier, both in the array declaration and its usage (the * has to touch the array to get the l' prefix to work):

! array pronoun = le |la |les |l'

+ allume @pronoun*
- triggger

The other tricky thing with having literal spaces in the array is that you can't have the final item in the array have a space at the end, because the lines of text get stripped of whitespace during parsing (same goes for if you wanted a prefix space as the first array element... after the name and value are separated out by the = sign, the value is stripped of whitespace from both ends).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants