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

ChatTrigger: Completely rework the criteria and parameter systems #122

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

camnwalter
Copy link
Member

@camnwalter camnwalter commented Apr 13, 2024

This removes the Parameter class and replaces it with 3 methods: startsWith, contains, and endsWith. These methods all take in criterias to make the API more understandable to users (in the past, I've noticed a lot of people attempting to do setContains(msg) instead of setCriteria(msg).setContains()).

setCriteria will work as normal, with a few slight tweaks.

  • Now global flags will work if the criteria is passed as a Regex
  • . or criteria variables will now detect \n.
  • String criteria variables will now not be greedy. e.g. setContains("<${a}>") with a string of "<A> B>" will capture "A" in this string, instead of "A> B".

A few thoughts:

  • I'm thinking that the contains criteria maybe should be global by default when using string criteria

  • There should be some way to do a "contains or" instead of having every contains match. Right now contains() has to have all of the containing criterias match

    • containsAll(), containsAny(), containsNone() variants?
  • startsWithout, endsWithout?

This removes the `Parameter` class and replaces it with 3 methods: `startsWith`, `contains`, and `endsWith`. These methods all take in criterias to make the API more understandable to users (in the past, I've noticed a lot of people attempting to do `setContains(msg)` instead of `setCriteria(msg).setContains()`).

setCriteria will work as normal, with a few slight tweaks.
- Now global flags will work if the criteria is passed as a Regex
- `.` or criteria variables will now detect `\n`.
- String criteria variables will now not be greedy. e.g. `setContains("<${a}>")` with a string of "<A> B>" will capture "A" in this string, instead of "A> B".
@mattco98
Copy link
Member

mattco98 commented Apr 13, 2024

What are the breaking changes here? I like the idea, but this is an extremely hot area of code, and can cause a massive amount of breakage which I'd like to avoid if possible. For example, I think the criteria greediness should be opt-in, e.g., something like .setContains("<${a?}>"), and same with the newlines.

@camnwalter
Copy link
Member Author

What are the breaking changes here? I like the idea, but this is an extremely hot area of code, and can cause a massive amount of breakage which I'd like to avoid if possible. For example, I think the criteria greediness should be opt-in, e.g., something like .setContains("<${a?}>"), and same with the newlines.

Breaking changes:

  • remove setParameter/setParameters/addParameter/addParameters, setChatCriteria, setExact, setStart, setEnd, triggerIfCanceled (never saw any module use this), setContains, setCaseInsensitive (also never saw anyone use these)
  • Adds contains, startsWith, and endsWith
    The biggest breaking change here would be the setContains, as I really haven't seen any of those other methods used at all. setCriteria will work the same as it did before.

The greediness opt-in sounds good to me, but what kind of symbol should we use to denote capturing newlines? Or would we have a boolean second param or something.

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.

2 participants