-
Notifications
You must be signed in to change notification settings - Fork 81
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
Improve Query and SearchResults #46
Conversation
It will also close #45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes!
Some questions:
- are there any tests for the search method but with optional parameters (
attributesToCrop
,facetsDistribution
...)? If not, can you add some of them? 🙂 - Do your changes involve modification in this PR Add code-sample file #40?
I put the breaking-changes
flag since your changes seem to be breaking! (this flag will automatically increase the minor number of the version instead of the patch one)
/// The object that contains information about the matches. | ||
#[serde(rename = "_matchesInfo")] | ||
pub matches_info: Option<HashMap<String, Vec<MatchRange>>> | ||
} | ||
|
||
#[derive(Deserialize, Debug)] | ||
#[serde(rename_all = "camelCase")] | ||
/// A struct containing search results and other information about the search. | ||
pub struct SearchResults<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could make it a serde_json::Value
when the user don't specify it?
On the SearchResults
or SearchResult
structs, I don't know, what do you think about that?
struct SearchResults<T: Value> {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if it would be useful in some cases. But is this possible without making a new method ? If yes we could do it anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand, why would it need a new method? The T
type must implement Ser/Deserialize
so it works with serde_json::Value
if the user doesn't specify the T
type itself.
Ask me any question, I am not sure my message above helps much, and also, this is not mandatory to implement.
Yes it's a breaking change so there will be some things to change in the sample file @curquiza |
Okay, @Mubelotix, but it should be done soon in case there is an error. For the momebt, I have no proof it works without any test. |
Have all the changes asked @Kerollmops been taken into account? Is this PR ready for merging? |
Change Vecs to &[]
213c2b9
to
69f90c6
Compare
Let's merge fast so we can merge #40 too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work 😄 But maybe you could chnage the builder a little bit!
Co-authored-by: Kerollmops <clement@meilisearch.com>
bors try |
tryBuild failed: |
@Mubelotix, the tests fail when you make your branch up to date with master. It will be impossible to merge even if Kero approves your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for these changes, I started to complain about the build
, execute
and execute_query
but it is not that bad in fact.
I think the Index::execute_query
is here just to be able to use the same query on multiple indexes 👍
bors try |
tryBuild succeeded: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
bors merge |
Build succeeded: |
Will close #41
TODO
Query
Results