-
Notifications
You must be signed in to change notification settings - Fork 57
Bump meilisearch-ruby (v0.18.0) #99
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
Conversation
|
This PR is breaking because uses meilisearch-ruby v0.18.0 that only works with MeiliSearch v0.25.0 and later -> it enforces the users to upgrade their MeiliSearch version |
80fffc3 to
c024e75
Compare
spec/integration_spec.rb
Outdated
| before_save_statuses = People.index.get_all_update_status | ||
| before_save_status = before_save_statuses.last | ||
| before_save_statuses = People.index.tasks['results'] | ||
| before_save_status = before_save_statuses.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, the most recent tasks are at the top of the list. In meilisearch v0.24.0 they were at the end of the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using the find method in this array in order to keep this working even when the order changes?
before_save_status = before_save_statuses.find {|t| t['...'] = ... }
And also for the line below...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure to get exactly what you want. Can you suggest changes? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use a expect/receive but I didn’t know for sure what method is responsible to update the docs, (and the one I found is private) so the other way is to just check by the size of results list:
it 'does not call the API if there has been no attribute change' do
person = People.search('Jane').first
expect {
person.update(first_name: 'Jane')
}.to_not change(People.index.tasks['results'], :size)
expect {
person.update(first_name: 'Alice')
}.to change(People.index.tasks['results'], :size).by(1)
endDo you think this could help covering this case? 🎱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me to cover the test case. I even remove the second part since the test was doing twice the same test.
c024e75 to
8b3a9d7
Compare
8b3a9d7 to
d782c98
Compare
brunoocasali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some ideas regarding these changes, let me know if you agree with them!
🥡🍕🐟 for you haha!
spec/integration_spec.rb
Outdated
| before_save_statuses = People.index.get_all_update_status | ||
| before_save_status = before_save_statuses.last | ||
| before_save_statuses = People.index.tasks['results'] | ||
| before_save_status = before_save_statuses.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using the find method in this array in order to keep this working even when the order changes?
before_save_status = before_save_statuses.find {|t| t['...'] = ... }
And also for the line below...
brunoocasali
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
get_or_create_indexmethod usage