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.

Sign up for GitHub

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

Jump to bottom

Refactorise the errors #234

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Refactorise the errors #234

merged 1 commit into from
Feb 11, 2022

Conversation

irevoire
Copy link
Member

  • Merge the two meilisearch error types
  • Use Deserialize and Serialize to generate the string associated with every error instead of writing it by hand

@irevoire irevoire requested a review from Kerollmops February 10, 2022 15:44
Kerollmops
Kerollmops previously approved these changes Feb 10, 2022
View reviewed changes
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Looks very good to me. Implementing serde on error highly reduced the amount of code to maintain, that's nice!

@irevoire irevoire requested a review from curquiza February 10, 2022 16:03
@curquiza
Copy link
Member

@irevoire is it breaking for the users?

@irevoire
Copy link
Member Author

irevoire commented Feb 10, 2022 •
edited
Loading

Huum when the user is accessing the error contained in a task he'll get types on the error_code and error_type instead of getting everything as string.
I think that's the only change for the user.
So yes it's kinda breaking.

@curquiza curquiza changed the title refactorise the errors Refactorise the errors Feb 10, 2022
@curquiza curquiza added the breaking-change The related changes are breaking for the users label Feb 10, 2022
@irevoire
Copy link
Member Author

I rebased on main, you can merge

@brunoocasali brunoocasali self-requested a review February 10, 2022 23:53
@brunoocasali
Copy link
Member

bors merge

@brunoocasali
Copy link
Member

I rebased on main, you can merge

I'll release it tomorrow :)

@bors
Copy link
Contributor

bors bot commented Feb 11, 2022

Build succeeded:

@bors bors bot merged commit 8ca3923 into meilisearch:main Feb 11, 2022
@brunoocasali brunoocasali mentioned this pull request Feb 16, 2022
Merged
bors bot added a commit that referenced this pull request Feb 16, 2022
@bors @brunoocasali
Merge #243
d4813d1 
243: Update version for the next release (v0.15.0) r=brunoocasali a=brunoocasali

## ⚠️ Breaking changes

* Refactorise the errors, now `error_code` and `error_type` has types instead of `string` (#234) `@irevoire`
* Put type on timestamp and duration (#237) `@irevoire`
  * The `time` crate will be needed if you need to manipulate API keys (check for more info #226, #237 (comment), and meilisearch/integration-guides#121 (comment)).

## 🚀 Enhancements

* Refactorise the settings (#235) `@irevoire`

Thanks again to `@irevoire!` 🎉


Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunoocasali brunoocasali brunoocasali approved these changes

@Kerollmops Kerollmops Kerollmops left review comments

@curquiza curquiza Awaiting requested review from curquiza

Assignees
No one assigned
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@irevoire @curquiza @brunoocasali @Kerollmops