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

Create get_update fn to get one update status #133

Merged
merged 6 commits into from
May 26, 2021

Conversation

icyJoseph
Copy link
Contributor

@icyJoseph icyJoseph commented May 10, 2021

Hi,

Having fun using this SDK, but I reckon a function to get the status of one single update is missing.

When I read through https://docs.meilisearch.com/reference/api/updates.html#get-an-update-status, I see for example:

 client.index('movies').getUpdateStatus(1)

Where the argument of getUpdateStatus, I guess, is the update id based on what the cURL version does:

curl \
  -X GET 'http://localhost:7700/indexes/movies/updates/1'

Which mirrors the MeiliSearch /indexes/:index_uid/updates/:updateId endpoint.

The rather unlucky thing is that to do the same with the Rust SDK it is not sufficient, not possible, to use the update id.

 let status: Status = progress.get_status().await.unwrap();

The above requires access to the progress instance, which is not possible in my case. Regardless of any specific use case, it is my opinion that for completeness sake, the get_update function, defined in this PR, is needed to complete the SDK. Of course this can be challenged, or perhaps there's a reason to why such function does not exist.

Thanks for the SDK! Great Job!

@Mubelotix
Copy link
Collaborator

Thank you very much! I thought about this when I created the library but that wasn't the priority at this time. I really like your solution. I will take a deeper look and give a probably favorable review in the next few days.

@curquiza curquiza changed the title feat: Create get_update fn to get one update status Create get_update fn to get one update status May 11, 2021
@curquiza
Copy link
Member

curquiza commented May 11, 2021

Hello @icyJoseph! Thanks a lot for your PR :)

Could you update the code sample file with the new usage of the function?

get_update_1: |-
let status: Status = progress.get_status().await.unwrap();

This file is used to generate the Rust tab in the official docs

@icyJoseph
Copy link
Contributor Author

Hello @icyJoseph! Thanks a lot for your PR :)

Could you update the code sample file with the new usage of the function?

get_update_1: |-
let status: Status = progress.get_status().await.unwrap();

This file is used to generate the Rust tab in the official docs

Will do!

src/progress.rs Outdated Show resolved Hide resolved
src/indexes.rs Outdated Show resolved Hide resolved
src/indexes.rs Outdated Show resolved Hide resolved
src/indexes.rs Outdated Show resolved Hide resolved
src/indexes.rs Outdated Show resolved Hide resolved
References the possiblity to get progress status directly on
the progress object. Narrow down the use cases for `get_update`.

On the `.code-samples.meilisearch.yaml` file, use two examples,
one showing how to get the status on the progress object, and a
second example showing how to get the update status from the index
using the update_id.

Also resolve other PR comments.
@icyJoseph icyJoseph requested review from Mubelotix and curquiza May 24, 2021 19:12
@curquiza
Copy link
Member

bors try

bors bot added a commit that referenced this pull request May 25, 2021
@bors
Copy link
Contributor

bors bot commented May 25, 2021

try

Build succeeded:

@icyJoseph
Copy link
Contributor Author

What happened to the wasm-build? is that done through another action?

@curquiza
Copy link
Member

curquiza commented May 25, 2021

What happened to the wasm-build? is that done through another action?

I removed this test from the settings, my bad 🙂 it was related to #138 that is not merged yet

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

I approve, but I wait for the @Mubelotix approval before any merge 🙂
Thanks again for this PR @icyJoseph

Copy link
Collaborator

@Mubelotix Mubelotix left a comment

Choose a reason for hiding this comment

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

I approve too!

@curquiza
Copy link
Member

Awesome!!
Thanks to both of you!

bors merge

@bors
Copy link
Contributor

bors bot commented May 26, 2021

Build succeeded:

@bors bors bot merged commit 6fa9fe4 into meilisearch:main May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants