Skip to content

Create an example showing how to update the Settings #245

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

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

irevoire
Copy link
Member

Pull Request

What does this PR do?

Provide an example that shows how to use the settings.

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@irevoire irevoire mentioned this pull request Feb 24, 2022
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Hey, just left some comments, trying to sane my doubts here!

let client = Client::new("http://localhost:7700", "masterKey");

// We try to create an index called `movies` with a primary_key of `movie_id`.
let my_index: Index = client
Copy link
Member

Choose a reason for hiding this comment

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

In line 8, you don't define the "type" but here you defined it, is this a convention or just a non-required thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, that's useless!
I copy-pasted some code and forgot to remove it.

But now you talk about it, I think it would be good to show all the types we're using, so when someone looks at the example, he knows instantaneously which function returns what without even looking at the doc.

I'll update my PR thanks 👍

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! This is something I need to be used to, in some communities like in the C# they are going to the full var style (without explicit types). Btw there is a widely used decision regarding the theme?

he knows instantaneously which function returns what without even looking at the doc.

I like this! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep usually in rust you elide all the types you can.
If the compiler is able to infer something let it do it.
Sometimes you even write « half » a type like that:

let chars: Vec<_> = "a string".chars().collect();

Here rust can't guess which kind of data structure you want to create with the .collect method so you need to say it's a Vec, but then you can elide the char because the .chars() method returns an iterator of char 😁

.unwrap()
// If the creation was successful we can generate an `Index` out of it.
.try_make_index(&client)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

I was reading some "scientific articles"(sarcasm) about unwrap.

My question is, can we use ? here? And can we avoid these calls or I just don't need to bother with that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, we can use ? when our function returns a Result.

A little rust lesson just so you know 😁 :

let result = task.try_make_index(&client);
let index = match result {
    Ok(index) => index,
    Err(error) => return Err(error.into()), // `.into` try to convert the error to the type the current function expect
};

This code is equivalent to:

let index = task.try_make_index(&client)?;

Ok so now you know 😁

My question is, can we use ? here? And can we avoid these calls or I just don't need to bother with that?

We could make our main return a Result<(), meilisearch_sdk::error::Error> (we can't return anything else than () in a main).
But in an example I THINK, we prefer to write the dumbest code possible to show every little step without any ambiguity.
Now that's my opinion and I can be wrong. There is no standard in rust about that.

Another thing we could do is call expect instead of unwrap. Expect allows us to write a little error message that could be more explicit for the user before unwrapping the error. Something like Can't reach the server..
Like here for example.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a lot for the detailed explanation you're the best! Now it makes sense to me (no, I'm not an expert yet but it makes sense to me haha).
I agree with you regarding the samples, being the simplest as possible. Experienced users will not how to make it simpler :)

add a bunch of comments to ease the understanding of the example

add more verbosity to the example
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Good job, thanks for your contributions here @irevoire ❤️

@brunoocasali
Copy link
Member

bors merge

@bors
Copy link
Contributor

bors bot commented Mar 2, 2022

Build succeeded:

@bors bors bot merged commit 0f2fb60 into meilisearch:main Mar 2, 2022
@irevoire irevoire deleted the settings-example branch April 20, 2022 13:50
@brunoocasali brunoocasali added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 25, 2022
bors bot added a commit that referenced this pull request Apr 25, 2022
272: Update version for the next release (v0.16.0) r=brunoocasali a=brunoocasali

### Release notes:

## ⚠️ Breaking changes

* Use an `OffsetDateTime` on the `ClientStats` (#244) `@irevoire`
* Fixed formatting with clippy & Removed `Document` trait (#267) `@irevoire`

## 🚀 Enhancements

* Add methods to automatically add/update documents in batches (#262) `@abhizer`
* Feature/Tenant Token (#263, #264) `@brunoocasali`

## Misc

* Create an example showing how to update the Settings (#245) `@irevoire`

Thanks again to `@abhizer,` `@bidoubiwa,`  `@brunoocasali,` `@irevoire,`  and Adrian Coutsoftides! 🎉


Co-authored-by: Bruno Casali <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants