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

Is this a useful feature to add? #1408

Closed
DEADB17 opened this issue Jan 14, 2018 · 14 comments

Comments

@DEADB17
Copy link
Contributor

commented Jan 14, 2018

When doing counse-ag searches I find it useful to be able to specify extra switches along with the search pattern. This allows to get interactive results instead of the two step process of calling counsel-ag with a prefix argument.

For example:

[dir] ag: --elisp --ignore site-lisp -- let

Searches for the pattern let only in emacs-lisp files ignoring the site-lisp directory.

So far I've been making these changes in my personal configuration, but if you think it's worth adding them to swiper I can create a PR.

Of course, if there is a better way to accomplish this let me know.
Thank you very much for swiper!

counsel-ag-function

(defun lr/parse-ag-input (string)
  "Parse the user input as STRING into a list of two strings: (query extra-ag-args).
Where query is the search term and extra-ag-args are any additional switches."
  (let ((query string)
        (extra-ag-args ""))
    (when (string-prefix-p "-" string)
      (let ((index (search "-- " string)))
        (when index
          (setq query (substring string (+ 3 index)))
          (setq extra-ag-args (concat " " (substring string 0 index))))))
    (list query extra-ag-args)))

(defun lr/counsel-ag-function (original/counsel-ag-function &rest args)
  (let* ((string (car args))
         (base-cmd (cadr args))
         (extra-ag-args (caddr args))
         (result (lr/parse-ag-input string)))
    (setq string (car result))
    (setq extra-ag-args (concat extra-ag-args (cadr result)))
    (funcall original/counsel-ag-function string base-cmd extra-ag-args)))

(advice-add 'counsel-ag-function :around #'lr/counsel-ag-function)

counsel-ag-occur

(defun lr/counsel-grep-like-occur (cmd-template)
  (unless (eq major-mode 'ivy-occur-grep-mode)
    (ivy-occur-grep-mode)
    (setq default-directory counsel--git-dir))
  (setq ivy-text
        (and (string-match "\"\\(.*\\)\"" (buffer-name))
             (match-string 1 (buffer-name))))
  (let ((result (lr/parse-ag-input ivy-text))
        (extra-ag-args nil)
        (cmd nil)
        (cands nil))
    (setq ivy-text (car result))
    (setq extra-ag-args (cadr result))
    (setq cmd (format cmd-template
                      extra-ag-args
                      (shell-quote-argument
                       (counsel-unquote-regex-parens
                        (ivy--regex ivy-text)))))
    (message "%s" cmd)
    (setq cands (split-string (shell-command-to-string cmd) "\n" t))
    ;; Need precise number of header lines for `wgrep' to work.
    (insert (format "-*- mode:grep; default-directory: %S -*-\n\n\n"
                    default-directory))
    (insert (format "%d candidates:\n" (length cands)))
    (ivy--occur-insert-lines
     (mapcar
      (lambda (cand) (concat "./" cand))
      cands))))

(defun lr/counsel-ag-occur ()
  "Generate a custom occur buffer for `counsel-ag'."
  (lr/counsel-grep-like-occur
   "ag --nocolor --nogroup %s -- %s"))

(advice-add 'counsel-ag-occur :override #'lr/counsel-ag-occur)
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jan 23, 2018

Sorry for the late reply. Could you explain how this is supposed to work interactively, maybe with some example using this repo?

C-u counsel-ag will ask you for ag arguments already. With your patch, I see the prompt changes, but I don't get what else.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

No worries.

Yes, it's not a new capability but a UI enhancement.
By including the ag arguments in the prompt, along with the search term it is possible to refine the search by changing the arguments without leave the prompt.

The prompt behaves as it does now unless the search term starts with --.
If it starts with -- the prompt interprets the input as arguments until it finds a -- (note the space at the end).

Sample interaction:
Suppose the user wants to search for the string let in ~/.emacs.d

  1. counsel-ag -> [.emacs.d] ag: let
  2. Realize that there are too many matches.
  3. Edit the text without leaving the prompt to limit to elisp files ->[.emacs.d] ag: --elisp -- let
  4. Still too many matches; a lot are coming from site-lisp which is irrelevant in this case.
  5. Edit again to exclude it -> [emacs.d] ag: --elisp --ignore site-lisp -- let
  6. The results are good. C-c C-o for further manipulation.

I hope it makes more sense.
Again, not something that was impossible before, but I found this UI more convenient.
Let me know if it's make sense to add it. (And no offense if you don't see value in it).

@mattsawyer77

This comment has been minimized.

Copy link

commented Mar 15, 2018

If I'm not mistaken, I think this is how helm-ag works which IMHO is very convenient.

I tried to use the prefix argument as @abo-abo mentioned above for counsel-ag (C-u C-c p s s), but I ended up with this prompt:

[my repo appears here] Ag regexp search for: 

then no matter what I type and hit RET, I then get

Package 'ag' is not available

I'm an emacs/spacemacs neophyte and might be misunderstanding or doing something wrong?

Anyway, if this PR is acceptable, would be cool to see it also in counsel-rg.

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2018

@mattsawyer77 C-c p s s is bound to projectile-ag when it's installed (wich requires the ag package to work), and it looks like it's your case.
Try C-u M-x counsel-ag.

@mattsawyer77

This comment has been minimized.

Copy link

commented Mar 16, 2018

thanks @DEADB17, that works. I think I prefer your method of passing args, though.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 19, 2018

@DEADB17 Sorry for the late reply; I've finally understood the value of having the args together with the search term. Could you please PR?

@kirill-gerasimenko

This comment has been minimized.

Copy link

commented May 4, 2018

That would be really great addition! Helm-ag already has this functionality and it's extremely convenient.
Please @DEADB17, add PR for this :)

I hope it also works with counsel-rg

@DEADB17 DEADB17 referenced this issue May 6, 2018
5 of 7 tasks complete
@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2018

There is an open PR #1559 that could use some testing and critique is anyone is interested 😄

@ghost

This comment has been minimized.

Copy link

commented May 8, 2018

@DEADB17 This looks pretty sweet! Thanks for your work.

@d1egoaz

This comment has been minimized.

Copy link

commented May 30, 2018

nice!!!! please merge this, just tested with rg and worked for me!

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 3, 2018

Of course, if there is a better way to accomplish this let me know.

I see no-one has mentioned counsel-git-grep and its counsel-git-grep-switch-cmd functionality added in #420. To summarise, typing C-cC-m during counsel-git-grep completion enters a recursive edit which allows you to modify the git grep command being used before returning to the updated git grep completion session.

I realise that Counsel, even more so than Ivy regexp builders, is a loose collection of community- (and owner-) contributed functionality (which no single person can easily maintain alone any more). Nevertheless, the counsel-ag/counsel-rg/counsel-grep/counsel-git-grep/counsel-pt/counsel-ack/etc. search commands seem to be some of the more fundamental and popular Counsel features (correct me if I'm wrong). Furthermore, AIUI they all call nearly interchangeable CLIs. As such, I strongly feel that Counsel ought to provide a complete and consistent interface to all of them. The incremental and independent means by which these commands have been added has resulted in some expected and (mis-)understandable Frankencode; see, for example, how rg and pt reuse ag definitions, ag reuses grep definitions, etc. This makes maintaining these interleaved commands, let alone extending them, even harder.

Personally, I think the ability to, in one way or another, modify the search command ad-hoc is essential. I like @DEADB17's suggestion to support command flags as part of the search input, as this is about as interactive/ad-hoc as it gets, with instant search result feedback. But the counsel-git-grep-switch-cmd approach has an advantage over this: it displays and allows editing of the full, unadulterated command, including options one would rarely specify interactively, such as the Git flag --no-pager. So, my opinion is that, where possible, Counsel should offer both flag-changing interfaces for the aforementioned search commands.

WDYT?

@DEADB17

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

Recursive editing is an interesting idea worth exploring even for the sake of consistency with other counsel commands, as you said in your comment.

It can either complement or replace the -- inline switches. It's hard to guess how a UI feels without trying it first IMO.

Regarding the additional complexity of the code, again it's a judgment call once we can get a feel of how useful or redundant the interaction between the two is.

In general I find that reducing complexity comes from observing common patterns emerge. Sometimes two features might look redundant initially but evolve into separate entities after a while.

So, I'm in favor of adding features, observing and simplifying once it can be rationalized.
Of course, I'm not the one who has to pay the cost of the extra complexity moving forward, so the final work should be up to the maintainer.

Regardless it'd interesting to try the recursive editing option. I don't have the time right now, but I will at some point if no one steps up before.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2018

It can either complement or replace the -- inline switches.

Judging from my own experience and the discussion in this issue, I don't think the recursive edit is convenient enough to replace inline switches. Its use case is more elaborate full-length command editing, so my suggestion is that they complement each other to an extent. Even if they are or become overly similar, there's no need to remove one or the other without good reason. Emacs and its packages should support as many workflows/options as possible within reason.

Regarding the additional complexity of the code, again it's a judgment call once we can get a feel of how useful or redundant the interaction between the two is.

The two UIs are mostly, if not completely, independent of one another (one has already existed, and #1559 is proposing the addition of the other). So, agreeing that these two UIs are sufficient and should be applied consistently to all Counsel search commands will create no greater complexity than already exists.

Regardless it'd interesting to try the recursive editing option. I don't have the time right now, but I will at some point if no one steps up before.

Steps up to do what? Port counsel-git-grep-switch-cmd to all aforementioned Counsel search commands?

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 14, 2018

As such, I strongly feel that Counsel ought to provide a complete and consistent interface to all of them

Agreed.

So, my opinion is that, where possible, Counsel should offer both flag-changing interfaces for the aforementioned search commands.

WDYT?

Also agree. I think inline flags might be more convenient though.

Judging from my own experience and the discussion in this issue, I don't think the recursive edit is convenient enough to replace inline switches.

It can't replace it, but it's a simple low-impact command / key binding that can do the job. Plus it has history, which might be useful.

@abo-abo abo-abo closed this Jun 19, 2018

DEADB17 added a commit to DEADB17/swiper that referenced this issue Aug 4, 2018
abo-abo added a commit that referenced this issue Aug 13, 2018
Interactively specify switches
Example for `counsel-rg' in this repo:

        -g*.el -- require
        -g*.org -- require
        -g!*.el -- require

Fixes #1408
Fixes #1512
Fixes #1688
Fixes #1559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.