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

Improve ranking rules error message #2407

Closed
juchom opened this issue May 18, 2022 · 5 comments · Fixed by #2468
Closed

Improve ranking rules error message #2407

juchom opened this issue May 18, 2022 · 5 comments · Fixed by #2468
Labels
bug Something isn't working as expected good first issue Good for newcomers milli Related to the milli workspace v0.28.0 PRs/issues solved in v0.28.0
Milestone

Comments

@juchom
Copy link

juchom commented May 18, 2022

Describe the bug
When changing the rules, the error message is misleading. According to the code :

let expected_error = json!({
"message": r#"`manyTheFish` ranking rule is invalid. Valid ranking rules are Words, Typo, Sort, Proximity, Attribute, Exactness and custom ranking rules."#,
"code": "invalid_ranking_rule",
"type": "invalid_request",
"link": "https://docs.meilisearch.com/errors#invalid_ranking_rule"
});

Accepted rules are Words, Typo, Sort, Proximity, Attribute, Exactness in fact accepted rules are words, typo, sort, proximity, attribute, exactness

To Reproduce
Try sending a rule with a capital letter

Expected behavior
The rule name is case insensitive and the endpoint needs a fix and the error message is correct.

The rule name is case sensitive and the error message needs a fix to match this requirement.

Meilisearch version: [e.g. v0.27.1]

@curquiza curquiza added good first issue Good for newcomers bug Something isn't working as expected milli Related to the milli workspace labels May 18, 2022
@curquiza
Copy link
Member

curquiza commented May 18, 2022

Hello @juchom

Thanks for this report, I put it as good first issue. The change should be done on the milli's repo.
For any person who wants to contribute: feel free to ask any question or help 😄

PS: nice choice of test, @ManyTheFish can be proud 😇

@curquiza curquiza changed the title Fix error message Improve ranking rules error message May 18, 2022
@YanivDan
Copy link

YanivDan commented May 23, 2022

hi
i think i found where to make the change is it in milli/milli/src/criterion.rs line 11?

option for a solution change the error message to :
#[error("{name} ranking rule is invalid. Valid ranking rules are Words, Typo, Sort, Proximity, Attribute, Exactness and custom ranking rules (with first capital letter). ")]

  • the change is marked and italic typed

if true, pls notify me i would like to take this one :)

@MarinPostma
Copy link
Contributor

hello @YanivDan, a PR has already been opened to address this issue here meilisearch/milli#536. Thanks for helping out!

@curquiza curquiza added this to the v0.28.0 milestone May 24, 2022
bors bot added a commit to meilisearch/milli that referenced this issue May 24, 2022
536: Improves ranking rules error message r=Kerollmops a=matthias-wright

This PR improves the ranking rules error message to properly reflect the case sensitivity.
The issue was highlighted in [meilisearch/issues/2407](meilisearch/meilisearch#2407).
Cheers!

Co-authored-by: Matthias Wright <matthias.s.wright@gmail.com>
@curquiza curquiza linked a pull request Jun 7, 2022 that will close this issue
4 tasks
@curquiza curquiza mentioned this issue Jun 7, 2022
4 tasks
bors bot added a commit that referenced this issue Jun 7, 2022
2468: Update milli 0.29 r=ManyTheFish a=ManyTheFish

- [x] Update milli to 0.29
- [x] Integrate charabia
- [x] Set disabled_words to default when Index::exact_words returns None
- [x] Fix ranking rules integration test

fixes #2375
fixes #2144
fixes #2417
fixes #2407

Co-authored-by: ManyTheFish <many@meilisearch.com>
bors bot added a commit that referenced this issue Jun 7, 2022
2468: Update milli 0.29 r=ManyTheFish a=ManyTheFish

- [x] Update milli to 0.29
- [x] Integrate charabia
- [x] Set disabled_words to default when Index::exact_words returns None
- [x] Fix ranking rules integration test

fixes #2375
fixes #2144
fixes #2417
fixes #2407

Co-authored-by: ManyTheFish <many@meilisearch.com>
bors bot added a commit that referenced this issue Jun 7, 2022
2468: Update milli 0.29 r=curquiza a=ManyTheFish

- [x] Update milli to 0.29
- [x] Integrate charabia
- [x] Set disabled_words to default when Index::exact_words returns None
- [x] Fix ranking rules integration test

fixes #2375
fixes #2144
fixes #2417
fixes #2407

Co-authored-by: ManyTheFish <many@meilisearch.com>
@MarinPostma
Copy link
Contributor

closed by meilisearch/milli#536

bors bot added a commit that referenced this issue Jun 8, 2022
2468: Update milli 0.29 r=irevoire a=ManyTheFish

- [x] Update milli to 0.29
- [x] Integrate charabia
- [x] Set disabled_words to default when Index::exact_words returns None
- [x] Fix ranking rules integration test

fixes #2375
fixes #2144
fixes #2417
fixes #2407

Co-authored-by: ManyTheFish <many@meilisearch.com>
bors bot added a commit that referenced this issue Jun 8, 2022
2468: Update milli 0.29 r=curquiza a=ManyTheFish

- [x] Update milli to 0.29
- [x] Integrate charabia
- [x] Set disabled_words to default when Index::exact_words returns None
- [x] Fix ranking rules integration test

fixes #2375
fixes #2144
fixes #2417
fixes #2407

Co-authored-by: ManyTheFish <many@meilisearch.com>
@curquiza
Copy link
Member

curquiza commented Jun 8, 2022

I re-open, we will close it once the fix is available in Meilisearch 😊
Will be closed by #2468

@curquiza curquiza reopened this Jun 8, 2022
bors bot added a commit that referenced this issue Jun 8, 2022
2468: Update milli 0.29 r=ManyTheFish a=ManyTheFish

- [x] Update milli to 0.29
- [x] Integrate charabia
- [x] Set disabled_words to default when Index::exact_words returns None
- [x] Fix ranking rules integration test

fixes #2375
fixes #2144
fixes #2417
fixes #2407

Co-authored-by: ManyTheFish <many@meilisearch.com>
bors bot added a commit that referenced this issue Jun 8, 2022
2468: Update milli 0.29 r=ManyTheFish a=ManyTheFish

- [x] Update milli to 0.29
- [x] Integrate charabia
- [x] Set disabled_words to default when Index::exact_words returns None
- [x] Fix ranking rules integration test

fixes #2375
fixes #2144
fixes #2417
fixes #2407

Co-authored-by: ManyTheFish <many@meilisearch.com>
@bors bors bot closed this as completed in #2468 Jun 8, 2022
@bors bors bot closed this as completed in 6171f17 Jun 8, 2022
@davelarkan davelarkan modified the milestones: v0.28.0, v0.29.0 Jul 27, 2022
@curquiza curquiza removed this from the v0.29.0 milestone Aug 23, 2022
@curquiza curquiza added this to the v0.28.0 milestone Aug 23, 2022
@curquiza curquiza added the v0.28.0 PRs/issues solved in v0.28.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected good first issue Good for newcomers milli Related to the milli workspace v0.28.0 PRs/issues solved in v0.28.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants