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

Made search case insensitive #2374

Merged
merged 21 commits into from
Jan 25, 2024
Merged

Made search case insensitive #2374

merged 21 commits into from
Jan 25, 2024

Conversation

Hetarth02
Copy link
Contributor

@Hetarth02 Hetarth02 commented Dec 10, 2023

This PR aims to fix Issue #2373.

image

@Hetarth02
Copy link
Contributor Author

@mortenpi Before merging, where do I add the entry in CHANGELOG.md? In version 1.2.1 or any other place?

@fredrikekre
Copy link
Member

Vim has smartcase which I have started to like. Basically, if the search query contains at least one upper case letter, then the search is case sensitive, otherwise not. Perhaps that is something to mimic here?

@fredrikekre
Copy link
Member

where do I add the entry in CHANGELOG.md? In version 1.2.1 or any other place?

Above 1.2.1 in a new header with ### Unreleased

@3f6a
Copy link

3f6a commented Dec 24, 2023

Vim has smartcase which I have started to like.

Why not a togglable setting for case-sensitivity? Seems more intuitive.

@LilithHafner
Copy link
Contributor

Honestly, this seems like an upstream bug in mini-search. I would hope that even a very light fuzzy search would fix this, but apparently they use Levenshtein edit distance to compute fuzzy matches which is not a case-aware system (i.e. Editlink and editlink are equally far apart as editWink and editlink).

source: https://lucaong.github.io/minisearch/types/MiniSearch.SearchOptions.html#__type.weights.__type-10.fuzzy-1

I think smartcase seems like a reasonable idea and it's also worth trying out fuzzy: true to see if minisearch's default fuzzy parameters are enough to fix this.

Copy link
Contributor

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

This seems like an unambiguous improvement. LGTM.

Smartcase is certainly an interesting idea to try out, but does not block this PR.

@mortenpi mortenpi linked an issue Jan 25, 2024 that may be closed by this pull request
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

LGTM as well, sorry for the delay. Thank you!

@mortenpi mortenpi enabled auto-merge (squash) January 25, 2024 01:33
@mortenpi mortenpi disabled auto-merge January 25, 2024 01:33
CHANGELOG.md Show resolved Hide resolved
@mortenpi mortenpi enabled auto-merge (squash) January 25, 2024 01:34
@mortenpi mortenpi added Type: Enhancement Format: HTML Related to the default HTML output labels Jan 25, 2024
@mortenpi mortenpi merged commit fd8155d into JuliaDocs:master Jan 25, 2024
18 of 21 checks passed
@Hetarth02 Hetarth02 deleted the Issue-2373 branch February 20, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Format: HTML Related to the default HTML output Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search should not be case-sensitive
5 participants