Add support for typo tolerance customization #351

vishalsodani · 2022-10-10T16:26:18Z

Pull Request

Related issue

Fixes #260

What does this PR do?

...

PR checklist

Please check if your PR fulfills the following requirements:

Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
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!

bidoubiwa

Hey @vishalsodani thanks for this contribution :)

Could you look at the specification? I think the parameters are optional

bidoubiwa · 2022-10-12T10:53:50Z

I know it was not part of the PR description, but could you add (or update) the following code-samples based on this example:

get_typo_tolerance_1
update_typo_tolerance_1
reset_typo_tolerance_1
typo_tolerance_guide_1
typo_tolerance_guide_2
typo_tolerance_guide_3
typo_tolerance_guide_4
settings_guide_typo_tolerance_1
getting_started_typo_tolerance
update_settings_1

vishalsodani · 2022-10-13T04:53:59Z

I know it was not part of the PR description, but could you add (or update) the following code-samples based on this example:
* get_typo_tolerance_1

* update_typo_tolerance_1

* reset_typo_tolerance_1

* typo_tolerance_guide_1

* typo_tolerance_guide_2

* typo_tolerance_guide_3

* typo_tolerance_guide_4

* settings_guide_typo_tolerance_1

* getting_started_typo_tolerance

* update_settings_1

ok. I will add these.

vishalsodani · 2022-10-13T09:04:06Z

Hey @vishalsodani thanks for this contribution :)

Could you look at the specification? I think the parameters are optional

Can you please review? I have made the parameters optional. Is the implementation ok?

bidoubiwa

Very nice PR 🔥

src/settings.rs

bidoubiwa · 2022-10-13T11:30:11Z

one_type and two_typos are u8 and not i64. See here

vishalsodani · 2022-10-16T07:16:09Z

Hi,

I feel there is an issue with this API design. If I tun this test, the test fails

`

     #[meilisearch_test]

     async fn test_set_typo_tolerance(client: Client, index: Index) {

    let typo_tolerance = TypoToleranceSettings {
        enabled: None,
        disable_on_attributes: Some(vec![]),
        disable_on_words: Some(vec![]),
        min_word_size_for_typos: Some(MinWordSizeForTypos {
            one_typo: Some(6),
            two_typos: Some(9),
        }),
    };
    let task_info = index.set_typo_tolerance(&typo_tolerance).await.unwrap();
    client.wait_for_task(task_info, None, None).await.unwrap();

    let res = index.get_typo_tolerance().await.unwrap();

    assert_eq!(typo_tolerance, res);
}

    left: `TypoToleranceSettings { enabled: None, disable_on_attributes: Some([]), disable_on_words: Some([]), min_word_size_for_typos: Some(MinWordSizeForTypos { one_typo: Some(6), two_typos: Some(9) }) }`,

   right: `TypoToleranceSettings { enabled: Some(true), disable_on_attributes: Some([]), disable_on_words: Some([]), min_word_size_for_typos: Some(MinWordSizeForTypos { one_typo: Some(6), two_typos: Some(9) }) }`', src/settings.rs:1609:9

We are setting enabled as None which should not be allowed. Am I correct?

vishalsodani · 2022-10-16T11:24:58Z

I know it was not part of the PR description, but could you add (or update) the following code-samples based on this example:
* get_typo_tolerance_1

* update_typo_tolerance_1

* reset_typo_tolerance_1

* typo_tolerance_guide_1

* typo_tolerance_guide_2

* typo_tolerance_guide_3

* typo_tolerance_guide_4

* settings_guide_typo_tolerance_1

* getting_started_typo_tolerance

* update_settings_1

Added.

vishalsodani · 2022-10-16T11:47:12Z

Hey @vishalsodani thanks for this contribution :)

Could you look at the specification? I think the parameters are optional

I have redesigned the API to use a builder pattern. So, a user is able to create a default object and can then set various attributes.

vishalsodani · 2022-10-17T14:26:59Z

@bidoubiwa Hi, I am done with all the changes. Please review. Thanks!

bidoubiwa

Hey @vishalsodani

We are setting enabled as None which should not be allowed. Am I correct?

Actually, you are right and just raised a bug in Meilisearch haha! See this spec

🔴 Sending a value with a different type than Boolean for the enabled field returns an bad_request error.

While I appreciate a lot that you made so many checks 🔥, They checks should be done at Meilisearch side and not ours. As, if we start going that path, we are basically re-writing the error handler of Meilisearch.

Meanwhile, for us, null is accepted until fixed by Meilisearch in a next release.

Can you remove:

default_as_true
default_one_typo_size
default_two_typos_size
is_true
is_default_for_one_typo
is_default_for_two_typos
is_min_word_size_object_default

vishalsodani · 2022-10-18T14:47:44Z

default_one_typo_size

ok. Should I also revert the builder pattern? It does not seem that useful now.

vishalsodani · 2022-10-18T14:51:37Z

Hey @vishalsodani

Actually, you are right and just raised a bug in Meilisearch haha! See this spec

Great!

bidoubiwa · 2022-10-18T14:53:35Z

Should I also revert the builder pattern? It does not seem that useful now.

Yes probably it is best to keep consistency with the other nested settings like pagination that are not using the builder pattern.

vishalsodani · 2022-10-18T15:51:47Z

@bidoubiwa

Can you remove:
Done

bidoubiwa

In your code-samples, could you only add the fields that are required and removed the ones that are not? Example in comment.

bidoubiwa · 2022-10-18T16:07:24Z

.code-samples.meilisearch.yaml

+  let typo_tolerance = TypoToleranceSettings {
+    enabled: Some(true),
+    disable_on_attributes: Some(vec!["title".to_string()]),
+    disable_on_words: Some(vec![])


Suggested change

disable_on_words: Some(vec![])

The fields that are not required should not be added to the samples. Normally this should not raise an error, right?

src/settings.rs

bidoubiwa · 2022-10-18T16:13:12Z

src/settings.rs

+        assert_eq!(typo_tolerance, res);
+    }
+
+    #[meilisearch_test]


Can you add a test where you only send a partial object ?
for example:

update only enabled

make the request, check if correct

let typo_tolerance = TypoToleranceSettings { enabled: Some(false), };

in the same test:

update only disabled_on_attributes

make the request, check if both enabled and disabled_on_attributes still have the correct changes.

I have to initialize all the fields of a struct. I cannot have a partial struct. Is it possible?

`

missing structure fields: - disable_on_attributes - disable_on_words - min_word_size_for_typos

`

Oh yeah... I mixed up the languages. My bad. I guess there that's the reason for adding the builder pattern... If you don't mind can you add it? With all the default values set to None?

Very sorry about the mixup. A lot of context switches between languages😅

If you look at this sample code https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d78d2a9f68131c270eadffb4df44d4a0; the changes I had made allowed for partial objects to be sent. The standard output shows -

Ok("{\"minWordSizeForTypos\":{\"oneTypo\":0},\"age\":10}")

If you look at this sample code https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d78d2a9f68131c270eadffb4df44d4a0; the changes I had made allowed for partial objects to be sent. The standard output shows -

Ok("{\"minWordSizeForTypos\":{\"oneTypo\":0},\"age\":10}")

@bidoubiwa Please see the above sample code shared on playground.

bidoubiwa · 2022-10-26T16:04:44Z

This message is sent automatically

Thanks again for contributing to Meilisearch ❤️
If you are participating in Hacktoberfest, and you would like to receive some gift from Meilisearch too, please complete this form.

bidoubiwa · 2022-10-26T16:05:06Z

Hey @vishalsodani
I'm a bit underwater right now. I gave you the hacktoberfest-accepted label in order to validate your PR for DO.

omid · 2023-05-11T08:57:28Z

What's the state of this PR?
I can also help to improve it if there is something missing?

We need this.

irevoire · 2023-05-15T13:23:50Z

src/settings.rs

+impl Default for MinWordSizeForTypos {
+    fn default() -> Self {
+        MinWordSizeForTypos {
+            one_typo: Some(5),
+            two_typos: Some(9),
+        }
+    }
+}


We shouldn't try to guess the default value of meilisearch by default, + we don't want to overwrite a previous call of the route.
Sending None is enough.

Suggested change

impl Default for MinWordSizeForTypos {

fn default() -> Self {

MinWordSizeForTypos {

one_typo: Some(5),

two_typos: Some(9),

}

}

}

impl Default for MinWordSizeForTypos {

fn default() -> Self {

MinWordSizeForTypos {

one_typo: None,

two_typos: None,

}

}

}

irevoire · 2023-05-15T13:25:59Z

src/settings.rs

+impl Default for TypoToleranceSettings {
+    fn default() -> Self {
+        TypoToleranceSettings {
+            enabled: Some(true),
+            disable_on_attributes: Some(vec![]),
+            disable_on_words: Some(vec![]),
+            min_word_size_for_typos: Some(MinWordSizeForTypos::default()),
+        }
+    }
+}


Once again, no reason to force some settings when creating the default value.

Suggested change

impl Default for TypoToleranceSettings {

fn default() -> Self {

TypoToleranceSettings {

enabled: Some(true),

disable_on_attributes: Some(vec![]),

disable_on_words: Some(vec![]),

min_word_size_for_typos: Some(MinWordSizeForTypos::default()),

}

}

}

impl Default for TypoToleranceSettings {

fn default() -> Self {

TypoToleranceSettings {

enabled: None,

disable_on_attributes: None,

disable_on_words: None,

min_word_size_for_typos: None,

}

}

}

omid · 2023-05-15T15:33:21Z

@irevoire, the original author hasn't had any activity on GitHub since then.
Seems like somebody else needs to take control.

480: Add support for typo tolerance customization r=bidoubiwa a=omid It's coming after #351 PR. I fixed the comments of the other PR here. fixes #260 Co-authored-by: vishalsodani <[email protected]> Co-authored-by: Omid Rad <[email protected]> Co-authored-by: Omid Rad <[email protected]>

omid · 2023-06-12T10:43:34Z

It's been merged via the following PR: #480
@bidoubiwa let's close this also 🙏🏼

bidoubiwa · 2023-06-13T09:48:11Z

Absolutely :)

bidoubiwa self-requested a review October 12, 2022 09:18

bidoubiwa suggested changes Oct 12, 2022

View reviewed changes

vishalsodani added 3 commits October 13, 2022 11:21

resolve conflicts

da08977

make typotolerance setting fields optional

47c80a9

handle defaults

0674a1e

vishalsodani force-pushed the typotolerance_customization branch from 60667db to 0674a1e Compare October 13, 2022 05:53

add skip serializing attribute

f8b7f42

bidoubiwa reviewed Oct 13, 2022

View reviewed changes

src/settings.rs Outdated Show resolved Hide resolved

bidoubiwa added the enhancement New feature or request label Oct 13, 2022

make typo attribute size as u8

1a08e07

vishalsodani added 4 commits October 16, 2022 16:41

update code samples

8ae1629

update api design and tests

0673277

fix code samples

3e894ce

fix api and code sample

6a6abec

vishalsodani added 3 commits October 16, 2022 17:00

fix clippy warnings

566a686

fix fmt warnings

7e5396e

fix clippy warnings

d1a872a

fix code to handle skipping serializing default values

f12e8af

bidoubiwa suggested changes Oct 18, 2022

View reviewed changes

vishalsodani added 2 commits October 18, 2022 21:14

reverse all api changes

7f18c20

fixed code samples

161eca0

vishalsodani requested a review from bidoubiwa October 18, 2022 15:59

bidoubiwa suggested changes Oct 18, 2022

View reviewed changes

bidoubiwa added the hacktoberfest-accepted label Oct 26, 2022

bidoubiwa requested a review from irevoire November 8, 2022 15:51

curquiza removed the hacktoberfest-accepted label Nov 15, 2022

Merge branch 'main' into typotolerance_customization

d71b681

irevoire reviewed May 15, 2023

View reviewed changes

omid mentioned this pull request Jun 6, 2023

Add support for typo tolerance customization #480

Merged

bidoubiwa closed this Jun 13, 2023

Add support for typo tolerance customization #351

Add support for typo tolerance customization #351

Uh oh!

Conversation

vishalsodani commented Oct 10, 2022

Pull Request

Related issue

What does this PR do?

PR checklist

Uh oh!

bidoubiwa left a comment

Choose a reason for hiding this comment

Uh oh!

bidoubiwa commented Oct 12, 2022

Uh oh!

vishalsodani commented Oct 13, 2022

Uh oh!

vishalsodani commented Oct 13, 2022

Uh oh!

bidoubiwa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bidoubiwa commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vishalsodani commented Oct 16, 2022

Uh oh!

vishalsodani commented Oct 16, 2022

Uh oh!

vishalsodani commented Oct 16, 2022

Uh oh!

vishalsodani commented Oct 17, 2022

Uh oh!

bidoubiwa left a comment

Choose a reason for hiding this comment

Uh oh!

vishalsodani commented Oct 18, 2022

Uh oh!

vishalsodani commented Oct 18, 2022

Uh oh!

bidoubiwa commented Oct 18, 2022

Uh oh!

vishalsodani commented Oct 18, 2022

Uh oh!

bidoubiwa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bidoubiwa commented Oct 26, 2022

Uh oh!

bidoubiwa commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omid commented May 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omid commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

omid commented Jun 12, 2023

Uh oh!

bidoubiwa commented Oct 13, 2022 •

edited

Loading

bidoubiwa commented Oct 26, 2022 •

edited

Loading

omid commented May 15, 2023 •

edited

Loading