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

Allow keys to contain "is" and "?" #403

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

randomPoison
Copy link
Contributor

@randomPoison randomPoison commented Aug 18, 2024

Add support for having "is" in the key for memories, allowing us to save memories with names like "why is it going down". In order to support this, I've had to refactor the handler to change the order of the operations performed by the handler.

The reason that is necessary is because the current logic makes it so you can never retrieve a key that contains "is". For example, if we had a memory named "a is b", we would try to retrieve it with .rem a is b. But this will be interpreted as a request to create a new memory "a" with the value "b". To make it possible to have "is" in the key, we need to first check if the entire phrase matches an existing key. If it doesn't, we can then attempt to interpret it as a request to store a new key.

I also changed the handling of "?", allowing keys containing "?" to be remembered. The current logic will only use the text before the first "?", so if you tried to remember a key named "hello?" it would always instead search for a memory named "hello". This PR changes the logic to instead use the text up to the last "?". This means that searching for "hello?" will still look for a memory named "hello". But you can search for keys that contain a "?" by adding an extra "?" at the end, i.e. you'd do .rem hello?? to retrieve the key "hello?".

* Change the regex to strip the last "?" from the string, rather then stopping at the first "?".
* Use the stripped string in the final fallback case to make sure the final message correctly reflects the key that we attempted to look up.
@randomPoison
Copy link
Contributor Author

Also it might be a good idea to merge #384 before this one, that way I can also add tests to the rem tests script added in that PR.

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.

1 participant