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

Change search plugin regular expression to support non-latin characters #2242

Closed
alizadeh118 opened this issue Mar 22, 2020 · 8 comments · Fixed by #2283
Closed

Change search plugin regular expression to support non-latin characters #2242

alizadeh118 opened this issue Mar 22, 2020 · 8 comments · Fixed by #2283
Labels
contribution welcome Contributions welcome good first issue Good for newcomers has PR Has a related PR type: regression

Comments

@alizadeh118
Copy link
Contributor

alizadeh118 commented Mar 22, 2020

Feature request

What problem does this feature solve?

This add non-latin support for search plugin.

What does the proposed API look like?

It's not going to change API.

How should this be implemented in your opinion?

Now search plugin use this regex: (?=.*\bWORD\b) and it works for latin words but not for non-latin ones and this is about \b word boundaries.
We can use this alternative way to support both latin and non-latin characters:
(?=.*(?<=\s|^)WORD(?=\s|$))

Are you willing to work on this yourself?

Yes.

@meteorlxy
Copy link
Member

meteorlxy commented Mar 24, 2020

Introduced by #1557

Contribution welcome

@ThaddeusJiang
Copy link

hi @meteorlxy @alizadeh118
I have made a PR for this Issue.

@meteorlxy
Copy link
Member

@ThaddeusJiang

Hey, I just reviewed your solution. That may break current logic for English words.

I just made an alternative PR #2283 for this.

@ThaddeusJiang
Copy link

Hi @meteorlxy

As my test, nothing was broken.
Could you share your test case?

CleanShot 2020-04-06 at 15 01 24

@meteorlxy
Copy link
Member

@ThaddeusJiang

image

@ThaddeusJiang
Copy link

@meteorlxy Is it incorrect?

@meteorlxy
Copy link
Member

meteorlxy commented Apr 6, 2020

Before your change, it won't match ready. It's the logic for English word in #1557

@ThaddeusJiang
Copy link

Thank you, I understand that it didn't match ready before my change.

I read #1557, I view the improvement is

search can now take in multiple words separated by a space.

not relate fuzzy query.


BTW, Could we support fuzzy query?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution welcome Contributions welcome good first issue Good for newcomers has PR Has a related PR type: regression
Projects
None yet
3 participants