-
Notifications
You must be signed in to change notification settings - Fork 18
Add ValkeySearch commands #263
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
base: main
Are you sure you want to change the base?
Add ValkeySearch commands #263
Conversation
- Add generated Valkey Search commands from json doc files https://github.com/valkey-io/valkey-search/tree/main/src/commands Signed-off-by: Eric Musliner <eric.musliner@gmail.com>
Signed-off-by: Eric Musliner <eric.musliner@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #263 +/- ##
=======================================
Coverage ? 42.38%
=======================================
Files ? 98
Lines ? 12581
Branches ? 0
=======================================
Hits ? 5333
Misses ? 7248
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It's great you've added the updates to the spec files. I believe this'll also improve the documentation. I can't really merge this PR until the spec files PR is merged (and ideally in a release). We are near releasing a 1.0 and that will mean breaking changes can't be merged. We can workaround changes to the API caused by changes to the json spec files, but I don't really want to be doing this multiple times. We could hide these behind an "experimental-json-search" trait which we document as an API that may change. |
Completely understand. This PR is dependent on the JSON changes getting approved and merged in ValkeySearch. I definitely want eyes on that one to make sure it's correct and that I'm not missing anything. |
- Updated search-commands.json for updated ft.aggregate documentation - Add additional tests for ft.aggregate Signed-off-by: Eric Musliner <eric.musliner@gmail.com>
|
Updated again for ft.aggregate changes |
|
I'm wondering if we can release this behind a |
I've never used traits before, but the valkey search commands don't break the existing APIs so even if you do put out a 1.0.0 release before this gets merged in you could include the valkey search commands in a minor version update? Unless you're rationale is to bring it in as a trait since now since we are still waiting on confirmation about json docs? |
|
The rationale of adding it behind a trait tagged experimental is we are not committing to the public API. So if there is an error in the generated code we don't have to worry about breaking changes. |
That makes sense to me if you want to bring it in before the json documents are reviewed and merged. |
This is dependent on the work to update the command json files in the ValkeySearch project valkey-io/valkey-search#504