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

Make matches work with numerical value #1893

Merged
merged 1 commit into from
Nov 29, 2021
Merged

Conversation

Thearas
Copy link
Contributor

@Thearas Thearas commented Nov 10, 2021

Pull Request

What does this PR do?

Implement #1883.

I have test this PR with unit test. It appears to be working properly:
image

PTAL @curquiza

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?

@curquiza curquiza added this to the v0.25.0 milestone Nov 10, 2021
@curquiza curquiza linked an issue Nov 10, 2021 that may be closed by this pull request
@MarinPostma
Copy link
Contributor

I find it weird that we partially highlight a number. I think a number should be considered as one word @gmourier @curquiza

@gmourier
Copy link
Member

gmourier commented Nov 11, 2021

I find it weird that we partially highlight a number. I think a number should be considered as one word @gmourier @curquiza

We should keep the same behavior as on a string where MeiliSearch does not match the whole word as well. The highlighting feature consists of having markers to guide the user's eye into the details. I'm not convinced by having different highlighting behaviors between "types" as the default behavior since it could create more confusion in the end than being really helpful on a frontend side.

On the other hand, we could give the possibility in the future to define the behavior of the highlighting feature e.g. fully highlighted, or even how string or number values could behave in case of partial highlighting.

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.

It works!

Thanks for the PR @Thearas

⚠️ We need the @MarinPostma's review before any merge :)

@ManyTheFish
Copy link
Member

Hello @Thearas @MarinPostma!

This PR should fix the issue, thanks!
However, we know that will not highlight numbers perfectly, mainly because of the behavior of our current tokenizer.
We don't have the time to enhance the tokenizer before the beginning of 2022 and an issue with the label good first issue has been opened for this.

thanks again for your contribution!

Copy link
Contributor

@MarinPostma MarinPostma left a comment

Choose a reason for hiding this comment

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

thank you for your contribution :)

bors merge

@bors
Copy link
Contributor

bors bot commented Nov 29, 2021

Build succeeded:

@bors bors bot merged commit c9f3726 into meilisearch:main Nov 29, 2021
@Thearas Thearas deleted the matches-number branch November 29, 2021 14:58
@curquiza curquiza added the v0.25.0 PRs/issues solved in v0.25.0 label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v0.25.0 PRs/issues solved in v0.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make matches work with numerical value
5 participants