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

Add option for case sensitive inputs #246

Closed

Conversation

josephrexme
Copy link

new Rivescript({ caseSensitive: true }) would return inputs
without calling toLowerCase() on them. Fixes #245

@josephrexme
Copy link
Author

I narrowed this down and found out that I had called

unless @caseSensitive?

which means something different from what I intended

unless @caseSensitive is true

@kirsle
Copy link
Member

kirsle commented Sep 13, 2017

Can you add some unit tests for this?

@josephrexme
Copy link
Author

Done

@kirsle
Copy link
Member

kirsle commented Sep 13, 2017

I was hoping for a test that the non-wildcard parts of triggers still match correctly too. 😄

I checked out your branch to do some poking at it myself. It has a problem matching this trigger:

+ say *
- Umm... "<star>"

Output:

// on current master branch
You> say HellO WorlD!
Bot> Umm... "hello world"
You> SAY hello world
Bot> Umm... "hello world"

// on addCaseSensitiveInputs
You> say hello
Bot> Umm... "hello"
You> say HELLO
Bot> Umm... "HELLO"
You> SAY hello
Bot> That is interesting. Please continue.
You> Say HEllo World
Bot> Tell me more about that.

The regexps that match triggers should use the case insensitive flag. Around here is one place to update, and the %previous code might need to update too.

@josephrexme
Copy link
Author

josephrexme commented Sep 13, 2017

I guess that's the expected behavior. My goal wasn't to make the matchers case insensitive. It is to make the inputs case sensitive so if you have a SAY hello you should have a matcher that fits
+ SAY *. Else use without the caseSensitive option. So it'd be a tradeoff of the whole benefits from case insensitivity. I think trying to make an exception for the non-wildcards parts to remain case insensitive might 1. be over-engineering 2. lead to someone wanting the matchers case sensitive some day and proposing another option hence bloating the library. When people making the choice of staying case sensitive could just deal with it all that comes with it. Personally I had to just create optional non-wildcard options like:

+ (say|Say|SAY) *
- Umm... <star>

@josephrexme
Copy link
Author

As an update on this @kirsle I actually added the insensitive flag to those lines and a test for say hello HELLO kept failing with "No Reply Matched"

@josephrexme
Copy link
Author

josephrexme commented Oct 3, 2017

Negative. I didn't compile when I tested. It should be all good now. Also, I'm sorry I was after my own selfish need initially. Thought this through and saw why you requested it

@KAKwit
Copy link

KAKwit commented Dec 13, 2017

I have a specific circumstance where I need to ask the user to copy / paste an ID, which I'll then use to look up some info for them. The ID is unfortunately case sensitive. Since this change is a global thing, it wouldn't help. I'm wondering if there can be an option in the reply methods or something for specifying this, or perhaps a way to make a single topic case sensitive?

(I'm very new to this thing so apologies if this is a stupid question, but this scenario is a real world one and I'm thinking others would be likely to come across it as well).

@josephrexme
Copy link
Author

@kirsle I don't know if you've seen this yet. What do you think? I wonder why it's not been considered for merge. @KAKwit in the mean time, you could use rivescript-promises. With that you can make your user inputs case sensitive by passing it as a parameter to the Rivescript instance:

new Rivescript({ utf8: true, caseSensitive: true })

@kirsle
Copy link
Member

kirsle commented Jul 18, 2018

The upstream code has changed quite a bit now (moving from CoffeeScript back to vanilla JS); mind rebasing this PR on the latest master?

kirsle added a commit that referenced this pull request Oct 2, 2021
kirsle added a commit that referenced this pull request Oct 2, 2021
@kirsle
Copy link
Member

kirsle commented Oct 2, 2021

Hey guys, better late than never, I ported this PR onto the modern JavaScript base of rivescript-js and merged it in #378.

RiveScript.js v2.2.0 adds the caseInsensitive option to the constructor. Sorry I've neglected this for so long, thanks guys!

@kirsle kirsle closed this Oct 2, 2021
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

Successfully merging this pull request may close these issues.

Make lowerCase conversion optional
3 participants