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

Make counsel-ag respect ivy-re-builder-alist #1935

Closed
wants to merge 10 commits into from

Conversation

@CeleritasCelery
Copy link
Contributor

@CeleritasCelery CeleritasCelery commented Feb 13, 2019

closes #1342. counsel-ag nor its derivatives support out of order matching, even though the underlying programs do. This means builders like ivy--regex-ignore-order or ivy--regex-plus are essentially ignored. I took the liberty of enabling out of order matching for the searchers that support it. This is one of the killer features of helm-ag IMHO. This will respect ivy-re-builder-alist.

The only corner case for this is rg which did not add support for look-around's till 0.10.0 which was released Sep 2018. I implemented a solution to automatically handle the version checking, but if that seems too heavy handed I can remove it. Instead we could just define a custom variable like counsel-rg-look-around-enabled-p. However that would mean it would not work out of the box anymore.

Last thing I changed that may be up for debate is I removed the :caller options from counsel-ag. See #1934 for more details. I think this is the right call, because now you can set a different re-builder for each type of searcher, which was impossible before. If you still want to match all counsel-ag related functions, you can set the key to the collection (counsel-ag-function).

counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Feb 13, 2019

Thanks for working on this!

Last thing I changed that may be up for debate is I removed the :caller options from counsel-ag. See #1934 for more details. I think this is the right call, because now you can set a different re-builder for each type of searcher, which was impossible before. If you still want to match all counsel-ag related functions, you can set the key to the collection (counsel-ag-function).

As I mentioned in the linked issue, I don't think we should remove :caller from counsel-ag. It makes counsel-ag less customisable, it breaks existing user configurations, and it doesn't solve the real problem, which is that other search functions piggy-back counsel-ag in a very ad-hoc and messy way.

- remove shell code from defcustom
- simplify version checking
- remove predicate suffix from variables
- re-add caller to counsel-ag
@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Feb 14, 2019

Thank you for your review @basil-conto. I have pushed another commit based on your feedback. I did not change the wrappers around counsel-ag because that feels like a it should be tackled in a separate issue. Let me know what you think.

counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Feb 17, 2019

I was thinking about this some more, and I feel that I made it more complicated then it needed to be. I removed the custom variable and instead just check if rg doesn't produce an error when we use that flag. If accepts that flag, we assume that it works. Let me know what you think.

Copy link
Collaborator

@basil-conto basil-conto left a comment

LGTM.

counsel.el Outdated Show resolved Hide resolved
@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Mar 5, 2019

@abo-abo I now have a CA from gnu.org. If that was the hold up for merging this.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 7, 2019

Merged, thanks.

Please review my follow up commit with new added tests. I had to research what negative lookaheads were, since I didn't use them before. With the added tests, hopefully it should be more clear.

@CeleritasCelery CeleritasCelery deleted the ag-ignore-order branch Mar 7, 2019
@dsedivec
Copy link
Contributor

@dsedivec dsedivec commented Mar 12, 2019

Just a note for anyone using ivy--regex-ignore-order and ripgrep: this transformed searches such as foo to (?=.*foo) for me, which made ripgrep run about 20x slower—so slow that counsel-rg never finished on my work code base, even after several minutes.

If you have this problem, I suggest switching to another regexp builder for counsel-ag, such as ivy--regex-plus. Here's how I set this in my init.el:

(setf (alist-get 'counsel-ag ivy-re-builders-alist)
      #'ivy--regex-plus)

dsedivec added a commit to dsedivec/dot-emacs-d that referenced this issue Mar 12, 2019
@CeleritasCelery
Copy link
Contributor Author

@CeleritasCelery CeleritasCelery commented Mar 25, 2019

@dsedivec, note that ivy--regex-plus Will use look arounds if you attempt to add negative matching (e.g. foo ! bar). I would recommend using ivy--regex instead.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants