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

Update search docs #579

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

danFbach
Copy link
Contributor

Pull Request

Related issue

Fixes #577, references #578

What does this PR do?

  • Updates docs to better reflect how results can be consumed.
  • Some of the existing docs are wrong as SearchResult<T> result = await index.SearchAsync(...); is actually invalid since an explicit cast is required from ISearchable<T> to the type of result.
  • There was no context showing that the result should be cast to either SearchResult<T> or PaginatedResult<T> in order to populate properties EstimatedTotalHits or TotalHits and TotalPages respectively.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

If you have any critiques or anything you want added to the docs for this purpose, let me know. Just hoping to clarify how transform the interface correctly.

Technically the proper type could be derive by one of two methods:

var result = (SearchResult<T>)await index.SearchAsync<T>(...);

or

var result = await index.SearchAsync<T>(...);
if (result is SearchResult<T> searchResult)
{
    //...
}

I prefer the latter, so that's how i changed the docs.

@danFbach danFbach changed the title in response to #577 #578 update search docs in response to #577 #578 Nov 11, 2024
@danFbach
Copy link
Contributor Author

The .code-sample.meilisearch.yaml should also be updated, as there are many usages of

SearchResult<T> games = await client.Index("...").SearchAsync<T>("");

Which is now invalid since the addition of ISearchable.
Would you like me to include updates to .code-sample.meilisearch.yaml with this PR?

@curquiza
Copy link
Member

Would you like me to include updates to .code-sample.meilisearch.yaml with this PR?

Yes please, thank you 😊

… supported since ISearchable<T>. added context for casting ISearchable<T> into usable result
@danFbach
Copy link
Contributor Author

okay, I've updated the code samples. I added casting to the docs - let me know if this is out of scope, but i think it is useful for dotnet devs, as it was not immediately apparent to me before reviewing the source here.

@curquiza
Copy link
Member

If it's useful for dotnet dev, let's keep it. Thank you @danFbach 🙏

For more context: we do not have any .NET dev at meilisearch, so any .NET expertise is welcome!

README.md Outdated Show resolved Hide resolved
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@ahmednfwela if you have time to ensure you are ok with this good practice, it would be awesome 😊

@curquiza curquiza added the maintenance Anything related to maintenance (CI, tests, refactoring...) label Nov 27, 2024
@curquiza curquiza changed the title update search docs in response to #577 #578 Update search docs Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Anything related to maintenance (CI, tests, refactoring...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EstimatedTotalHits Removed
2 participants