-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
repair cask's fuzzy_search in search_casks
#13048
Conversation
@@ -36,8 +36,10 @@ def search_casks(string_or_regex) | |||
results = cask_tokens.extend(Searchable) | |||
.search(string_or_regex) | |||
|
|||
results |= DidYouMean::SpellChecker.new(dictionary: cask_tokens) | |||
.correct(string_or_regex) | |||
if results.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doing results |=
would be preferable to add more results even if there's only one. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doing
results |=
would be preferable to add more results even if there's only one. Thoughts?
I agree. I'll repair it.
.correct(string_or_regex) | ||
if results.empty? | ||
cask_names = Cask::Cask.all.map(&:full_name) | ||
results = DidYouMean::SpellChecker.new(dictionary: cask_names).correct(string_or_regex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
results = DidYouMean::SpellChecker.new(dictionary: cask_names).correct(string_or_regex) | |
results = DidYouMean::SpellChecker.new(dictionary: cask_names) | |
.correct(string_or_regex) |
@@ -48,7 +47,7 @@ def search_casks(string_or_regex) | |||
else | |||
cask.token | |||
end | |||
end | |||
end.uniq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end.uniq | |
end |
|=
will already ensure uniqueness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without uniq
, the search results will show duplicate cask names. Because result
will include the same cask's full_name
and one's cask_tokens
results.sort.map do |name| |
without uniq
brew search dockertoolbox
==> Formulae
docker-ls
==> Casks
docker-toolbox docker-toolbox
with uniq
brew search dockertoolbox
==> Formulae
docker-ls
==> Casks
docker-toolbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, in that case, what about doing +=
above given we're doing uniq
here anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MikeMcQuaid
Could you review this PR?
Thanks again @hyuraku! |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?related to #13028
repair Homebrew::Search::Extension.search_casks to use Cask.full_name instead of Tap.cask_tokens for fuzzy_search