meilisearch/meilisearch-rust

Extend Document trait with function to get index

amaihoefner opened this issue · 3 comments

Description
Add a index() function to the Document trait to make it easier to get access to the index.

Basic example
Right now, something that implments Into<String> needs to be provided when one wants to get access to an index:

let movie_index = client.index("movies");

With this change one could instead write:

let movie_index = Movie::index(&client);

(Provided Movie implements the Document trait)
This way accessing an index becomes less prone to typos and one would'nt have to keep track of the name of the index, only the associated struct.

Other
It might also make sense to add a constant INDEX_STR to the trait.
This would have the following benefits:

  • it could be used to provide a default implementation for the proposed Document::index function
  • it could be used when implementing Document::generate_index by hand

Adding this to the derive macro would also not require much effort, as the index name is already being extracted there.

I will provide a draft merge request with these changes to act as a base for further discussion.

Hey, I get why you would want to do that.
But a Document type should not be tied to an index.
You should be able to use the same type over multiple indexes.

wdyt @bidoubiwa?

Hm, I admit, I had not originally considered that.

But when we implement the Document trait for a type, don't we already create a sort of "binding" to a specific index because of the generate_index function of the trait?
And we would still be able to use the type (including the settings from generate_settings) for other indices (although the primary_key would not be included with those settings).

But yes, we may want to mention in the documentation of the proposed index function, that it refers to the same "default" index that generate_index is referring to.

Thinking about it, there might be a nice way of allowing multiple indices with a const generic str argument. So one could use something like Movie::index::<"my_index">(&client).
But those currently require nightly and the incomplete adt_const_params feature.
And that would work against the idea of this issue, by again requiring the index to be specified.
Of course, there might also be another, better way of accomplishing that.

Hey @amaihoefner
thanks for raising this issue.

But a Document type should not be tied to an index.

It is already the case, currently, the index_name is generated by snake_casing the Document structure's name. For example the Movie structure will have movie as an index_name.

This confusion is due to the trait being very badly named. It helps generate the settings based on the schema of the document. Thus it's more an Index configuration/schema than a Document.

My suggestion would be:
Rename the Document trait to IndexConfig or something better (if it's ok for you to do that in your PR).

In your PR, change

    fn index(client: &crate::client::Client) -> Index {

to

fn index(client: &Client) -> Index {