-
Notifications
You must be signed in to change notification settings - Fork 102
Bump meilisearch to v0.25.0 - New task API #225
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
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.
That is an amazing job that you've done here, especially this proc macro 🎉
Thank you very much for the time you took to improve this SDK!
run: cargo build --verbose | ||
- name: Meilisearch (latest version) setup with Docker | ||
run: docker run -d -p 7700:7700 getmeili/meilisearch:latest ./meilisearch --no-analytics=true --master-key=masterKey | ||
run: docker run -d -p 7700:7700 getmeili/meilisearch:latest ./meilisearch --no-analytics --master-key=masterKey |
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.
run: docker run -d -p 7700:7700 getmeili/meilisearch:latest ./meilisearch --no-analytics --master-key=masterKey | |
run: docker run -d -p 7700:7700 getmeili/meilisearch:latest ./meilisearch --no-analytics=true --master-key=masterKey |
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 yet, will be for v0.26.0, not v0.25.0 :)
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 am not sure that I understand that, the previous version of meilisearch works with this change, and the newer version (v0.26) will only work with this change. This patch makes the command line compatible with the future version of Meilisearch and doesn't break the v0.25 one.
Can't we accept this change?
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.
Hum from what I see it has been merged some times ago already; meilisearch/meilisearch#1984
And it works for me on meilisearch v0.25.2
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.
Thank you so much for this PR @irevoire ❤️
✨ Amazing work ✨
I left a few comments.
Also I didn't see the wait_for_task
function in the Index
can we do something like:
client.index("indexName").wait_for_task(1);
let t1 = index.set_filterable_attributes(["kind", "value"]).await?; | ||
let t2 = index.set_sortable_attributes(["title"]).await?; | ||
|
||
t2.wait_for_completion(client, None, None).await?; |
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 really like this name wait_for_completion
🥇 . You could do an issue to propose this new name 😊
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.
Awesome 🔥 🔥 🔥
LGTM!
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.
Could you remove the serialize lines, please? :)
oops yes sorry I thought I clicked on the commit suggestion button my bad. |
Update CONTRIBUTING.md Apply code suggestions Co-authored-by: Amélie <[email protected]>
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 Tamo! Thank you!
Perfecto! bors merge |
Build succeeded: |
We’ll do a little overview of what changed in this PR.
For the users:
Progress
type have been removed entirely.wait_for_pending_update
has been replaced by the methodwait_for_task
which takes a task uid or a task directly in parameter.wait_for_completion
has been implemented directly on the TaskFor the contributors:
MeiliSearch_test
macro for the unit tests only: https://github.com/meilisearch/meilisearch-rust/blob/41dd6dc765b7938ac399a12db2576b6ab9d479d1/meilisearch-test-macro/README.md