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

Add uFuzzy user customizable option: ufuzzyOptions #90

Closed
wants to merge 2 commits into from

Conversation

c0001
Copy link
Contributor

@c0001 c0001 commented Mar 28, 2023

Issued by #87, we should provide this to let multilingual searching be capable, robust and following user side specifications via doing more tweaks of uFuzzy(@leeoniya/ufuzzy).

See also:

Issued by Fannon#87, we should provide this to let multilingual searching be
capable, robust and following user side specifications via doing more
tweaks of uFuzzy(@leeoniya/ufuzzy).

See also:
- leeoniya/uFuzzy#14
- https://github.com/leeoniya/uFuzzy#how-it-works
Comment on lines +136 to +140
```yaml
# make CJK chars can be searched
ufuzzyOptions:
- interSplit: (p{Unified_Ideograph=yes})+
```
Copy link
Owner

Choose a reason for hiding this comment

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

Ok, I got the idea. Making this an option is actually very flexible. I just hoped that we could find a fix that "just works" and does not need user configuration. But if that is not realistic, we can think about options.

I see two approaches here:

  • Either we have a very simple option like supportCJK: true, which does the right thing for you.
  • Or we allow users to generally set uFuzzy options like you suggest. That is more flexible, but will only work for users that are comfortable with programming.

In case we go with the latter option, I'd propose to make it an object, not an array of objects:

Suggested change
```yaml
# make CJK chars can be searched
ufuzzyOptions:
- interSplit: (p{Unified_Ideograph=yes})+
```
```yaml
# make CJK chars can be searched
uFuzzyOptions:
interSplit: (p{Unified_Ideograph=yes})+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, object format is better for code and user side specification.

I placing the array format since I found the yaml option page saving
will automatically converts my inputs of object list to array in that
time that I haven't found a way for making the option setting using
yaml object format which caused me to follow the format type as what
the builtin customSearchEngines does.

Thanks for fix this, great!

* our builtin specifications. (See also
* https://github.com/leeoniya/uFuzzy/blob/main/src/uFuzzy.js#L9)
*/
ufuzzyOptions: [],
Copy link
Owner

Choose a reason for hiding this comment

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

like stated above, I think it would be better to make this an object, like in uFuzzy itself:

Suggested change
ufuzzyOptions: [],
ufuzzyOptions: {},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's also what I preferred to use. Thanks!

Fannon pushed a commit that referenced this pull request Mar 29, 2023
…CJK chars

Thanks to c0001 for providing original idea in PR #90
@Fannon
Copy link
Owner

Fannon commented Mar 29, 2023

I've just created a new PR which reverts the old change of mine and takes your approach of passing through uFuzzy options. This is how I would propose it: #91

@c0001
Copy link
Contributor Author

c0001 commented Mar 29, 2023

I've just created a new PR which reverts the old change of mine and takes your approach of passing through uFuzzy options. This is how I would propose it: #91

It's mature and perfect for me to adjusting my own needed fuzzy
features. So I close this old pull-request now, hope #91 will be
merged soon.

And further more that according to leeoniya's post, my self used
interSplit of (\\s|\\p{P}) may be useful for someone is first time
dating with the CJK fuzzy search problem of #87.

Thanks!
Briant

@c0001 c0001 closed this Mar 29, 2023
Fannon added a commit that referenced this pull request Mar 29, 2023
…CJK chars (#91)

Thanks to c0001 for providing original idea in PR #90

Co-authored-by: Simon Heimler <mail@simon-heimler.de>
@Fannon
Copy link
Owner

Fannon commented Mar 29, 2023

@c0001 : If you have any further insights how to best configure uFuzzy for your case, feel free to propose a better README fix or mention. In case we find a solution which works general-purpose, we can either have it in by default or if it comes with drawbacks, then have it as a dedicated option (like enableCJK: true)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants