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

Improve cash search #2471

Merged
merged 1 commit into from Jan 20, 2014
Merged

Conversation

voanhduy1512
Copy link
Contributor

Ignore all the hyphens in cask title and search term

@voanhduy1512
Copy link
Contributor Author

PR for #1189

@rolandwalker
Copy link
Contributor

Cool! I like this idea, and will test it, but note there is a merge conflict with #2461.

@voanhduy1512
Copy link
Contributor Author

Ok, i add ignore case to my PR like 2461.

@rolandwalker
Copy link
Contributor

That's also great!

Just be aware of a separate issue: GitHub can't smoothly merge in the pull request, because the code where you started from conflicts with what is now the current master branch. One solution is for you to pull down the current master, rebase these changes onto master, and push branch improve-cask-search back to GitHub.

However, if you have never done those operations before, I can do it for you if this PR is accepted. (The conflict originated with my commit, so I am happy to fix the side effects.)

@voanhduy1512
Copy link
Contributor Author

Maybe you should help me with this process. I tried to rebase and push back to my branch improve-cask-search on Github but didn't success. Thank you

@rolandwalker
Copy link
Contributor

You might have to give the -f argument to git push because you have changed the history. If you want detailed help you can find me on IRC as mentioned in HACKING.md. I have some comments on your code to appear shortly.

@rolandwalker
Copy link
Contributor

OK, I have tested this and I think it is cool. 👍

It does have one regression, though. Previously we accepted a regular-expression argument to search, and that is documented. Since hyphens are a valid part of regular expressions, this breaks:

brew cask search 'c[a-z]rome'

So I would propose we assume that an argument bounded by /slashes/ is a regexp, and continue to use the old logic in that case, like this

if search_term =~ %r{^/(.*)/$}
  search_regexp = $1
  casks = Cask::CLI.nice_listing(Cask.all_titles.grep(/#{search_regexp}/i))
else
  all_titles = Cask.all_titles
  ... <your new idea goes here> ...
end

Then, the advanced user can still do brew cask search '/c[a-z]rome/'

@voanhduy1512
Copy link
Contributor Author

All done.

@ghost ghost assigned rolandwalker Jan 20, 2014
Ignore all the hyphens in cask title and search term

make "brew cask search" ignore case like Homebrew#2461

restore search by regular expression feature
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants