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

Implement placeholder search. Update tests #71

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

eskombro
Copy link
Member

@eskombro eskombro commented Jul 21, 2020

About this PR:

As defined in meilisearch/meilisearch#771, MeiliSearch will implement placeholder search starting with v0.13.0

A search request containing no Query parameter (or a null value in that parameter) will send a response with a basic placeholder search showing ranked results, and will be as customizable as any other search request.

GO types and default initialization to zero value doesn't let us make a clear distinction between an uninitialized String (some nil value) and an empty String (some "" value) when all we are handling is a string. We need to be able to provide those two options in order to implement placeholder search, which expects no q parameter (no query) or a null value.

An option to do this would be using a string pointer that can be uninitialized (nil), contain an empty string, or contain a valid string. The problem with this is that it would add some unnecessary complexity to the whole search experience, as the user would need to always use pointers towards a string to perform the search. So adding a new functionnality results in making search experience work.

The most practical solution to this from my POV is to add a boolean value to the SearchRequest struct where the user indicates if he wants to perform a placeholder search. This won't be breaking, completey optional, and really simple to use.It can work as in:

resp, err = client.Search(indexUID).Search(SearchRequest{
	PlaceholderSearch: true,
})

Which would return the placeholder search results, and would be clearly different than

resp, err = client.Search(indexUID).Search(SearchRequest{
	Query: "",
})

if PlaceholderSearch is set to true, Query will be totally ignored for that request

Which would return no results

⚠️ Tests will not pass until v0.13.0 of MeiliSearch is released

Closes #69

@eskombro eskombro self-assigned this Jul 21, 2020
@eskombro eskombro changed the base branch from master to bump-meilisearch-v0.13.0 July 21, 2020 22:58
@eskombro eskombro requested review from alexisvisco and qdequele July 21, 2020 22:59
@eskombro eskombro force-pushed the implement_placeholder_search branch from 624ec9a to 9e915cf Compare July 22, 2020 00:08
Copy link
Member

@qdequele qdequele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about make like Algolia and write multiple functions for that?

resp, err = client.Search(indexUID).Search(SearchRequest{
    Query: "",
    ...SearchParameters
})
resp, err = client.Search(indexUID).PlaceholderSearch(PlaceholderSearchRequest{
    ...PlaceholderSearchParameters
})

@eskombro
Copy link
Member Author

Thanks @qdequele!

I agree with you and it seems like it would be the 'Go' way to handle this kind of situation. Talking with the integration team we also think that as a user it is easier to just play with one parameter in the request parameters object to be able to switch from 'normal search' to a 'placeholder search', and it might be annoying to be forced to use a different method with a different parameters object. So maybe we can leave for now the boolean variable to activate placeholder.

@eskombro eskombro merged commit 2cfaace into bump-meilisearch-v0.13.0 Aug 3, 2020
@eskombro eskombro deleted the implement_placeholder_search branch August 3, 2020 14:49
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.

Implement placeholder search and tests
2 participants