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

Improve error handler #65

Merged
merged 11 commits into from Jul 8, 2020
Merged

Improve error handler #65

merged 11 commits into from Jul 8, 2020

Conversation

curquiza
Copy link
Member

@curquiza curquiza commented Jul 5, 2020

Related to this issue meilisearch/integration-guides#19 and the new error handler available since the MeiliSearch v0.11.0

How to do a quick check

  • run a MS instance in local (with masterKey as master key)
  • create an index with test as UID
  • at the root of the repo, launch $ bundle exec ruby test.rb where test.rb is:
require 'meilisearch'

client = MeiliSearch::Client.new('http://127.0.0.1:7700', 'masterKey')
index_uid = 'test'

# To see the `details` method and the attributes when rescueing
begin
  client.create_index(index_uid)
rescue => e
  puts e.details
  puts e.type
  puts e.link
end

# To see the interpreter output
puts ''
client.create_index(index_uid)

@curquiza curquiza added the breaking-change The related changes are breaking for the users label Jul 5, 2020
@curquiza curquiza force-pushed the improve-error-handler branch 3 times, most recently from 44f7d12 to a77a6bf Compare July 5, 2020 14:30
@curquiza curquiza marked this pull request as ready for review July 5, 2020 14:30
@curquiza curquiza requested review from eskombro and tpayet July 5, 2020 14:31
@curquiza curquiza force-pushed the improve-error-handler branch 2 times, most recently from 13319b0 to 48ccb90 Compare July 5, 2020 14:41
@curquiza curquiza mentioned this pull request Jul 5, 2020
@curquiza curquiza force-pushed the improve-error-handler branch 2 times, most recently from 1cfc406 to 08ac4a5 Compare July 5, 2020 17:08
lib/meilisearch/error.rb Show resolved Hide resolved
lib/meilisearch/error.rb Show resolved Hide resolved
lib/meilisearch/error.rb Outdated Show resolved Hide resolved
lib/meilisearch/error.rb Outdated Show resolved Hide resolved
tpayet
tpayet previously approved these changes Jul 7, 2020
Copy link
Member

@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.

Amazing work and tests! Thanks!

Why are you using simple error naming? Shouldn't we use the naming chosen here instead of simplifying it?

e.g.
ApiError -> MeiliSearchApiError
CommunicationError -> MeiliSearchCommunicationError
TimeoutError -> MeiliSearchTimeoutError

@details = details

def initialize(message)
@message = "An error occurred while trying to connect to the MeiliSearch instance: #{message}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@message = "An error occurred while trying to connect to the MeiliSearch instance: #{message}"
@message = "An error occurred while trying to connect to MeiliSearch: #{message}"

Copy link
Member Author

@curquiza curquiza Jul 7, 2020

Choose a reason for hiding this comment

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

I wanted to make a real difference between the MeiliSearch search-engine/API/instance and the package the user is currently using that is also named "meilisearch" @eskombro
What do you suggest then?

Copy link
Member

Choose a reason for hiding this comment

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

👌

@curquiza
Copy link
Member Author

curquiza commented Jul 7, 2020

@eskombro

Why are you using simple error naming? Shouldn't we use the naming chosen here instead of simplifying it?

Because ruby allows namespaces. When the users will do a begin/rescue (try/catch equivalent), they will write rescue MeiliSearch::ApiError => e. The MeiliSearch is already present, and would be redundant if I rename all my errors with a MeiliSearch prefix (it would imply rescue MeiliSearch::MeiliSearchApiError => e).

I've already explained all of these in the issue you linked in the Remark section 😉

Some language allows namespacing. So the raised error could be MeiliSearch::ApiError instead of MeiliSearchApiError. It depends on the language. We should adopt the best practice of each language 🙂

For example, in the Ruby and PHP SDKs, we used the namespace MeiliSearch.
In the JS one, we use the prefix MeiliSearch instead, directly in the name of the error.

Also, depending on the language, the Error could be changed as Exception for example (ex: PHP).

And I don't need to write the namespace MeiliSearch:: in the internal code of this package because we work only in the generic module MeiliSearch (see the module MeiliSearch declared at the begining of each file).

Copy link
Member

@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.

👌

@curquiza curquiza merged commit 0121979 into master Jul 8, 2020
@curquiza curquiza deleted the improve-error-handler branch July 8, 2020 07:49
bidoubiwa pushed a commit that referenced this pull request Mar 8, 2021
* Use the new error handler of MeiliSearch API

* Improve the tests to check MS code and type

* Add comments

* Fix linter errors

* Add MeiliSearch::CommunicationError

* Upd rubocop_todo

* Remove useless byebug

* Change minor naming

* Remove useless error in get_index

* Update lib/meilisearch/error.rb

Co-authored-by: Thomas Payet <thomas@meilisearch.com>

* Update lib/meilisearch/error.rb

Co-authored-by: Thomas Payet <thomas@meilisearch.com>

Co-authored-by: Thomas Payet <thomas@meilisearch.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants