Skip to content

Conversation

curquiza
Copy link
Member

@curquiza curquiza commented Dec 2, 2020

No description provided.

@curquiza curquiza added the skip-changelog The PR will not appear in the release changelogs label Dec 2, 2020
@curquiza curquiza requested a review from eskombro December 2, 2020 17:44
Copy link
Contributor

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

Thanks for this huge work. 🎉

There is something that would be important to add to this (maybe in the PR or maybe in another one) which is checking the methods that raise errors.

  • Several of them say they raise an HTTPError but this has been upgraded with the SDK error handling. They will very often raise a MeiliSearchApiError instead of this.
  • Some of them do not mention raising an error even if they do.

This would be worth checking for making these comments and documentation better!

I also added a few suggestions :)

Thanks again

Copy link
Contributor

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

A suggestion for the description of the Error raised as MeiliSearchApiError:

I suggest:

        Raises
        ------
        MeiliSearchApiError
           An error containing details about why MeiliSearch can't process your request. MeiliSearch error codes are described here:
           https://docs.meilisearch.com/errors/#meilisearch-errors

Note that I changed not only the description, but the link in here. We used to link:
https://docs.meilisearch.com/references/#errors-status-code

I prefer linking this:
https://docs.meilisearch.com/errors/#meilisearch-errors

What do you think?

@curquiza curquiza requested a review from eskombro December 7, 2020 18:03
Copy link
Contributor

@eskombro eskombro left a comment

Choose a reason for hiding this comment

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

Thanks!!!!!!

🌮

@curquiza
Copy link
Member Author

curquiza commented Dec 7, 2020

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 7, 2020

Build succeeded:

@bors bors bot merged commit 3cc7536 into master Dec 7, 2020
@bors bors bot deleted the update-comments branch December 7, 2020 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog The PR will not appear in the release changelogs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants