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

[WIP] Add first implementation of consult-search #68

Closed
wants to merge 1 commit into from

Conversation

ackerleytng
Copy link

@ackerleytng ackerleytng commented Dec 15, 2020

Seeking comments on code style and implementation as I learn elisp!

Question: How do you develop on consult? Do you symlink this directory into your ~/.emacs.d?

@manuel-uberti
Copy link

Thank you so much for working on this. I feel like the first steps to a consult-rg command are on their way. :)

@manuel-uberti
Copy link

Instead of Projectile, couldn't you use the built-in project to get the project root?

@minad
Copy link
Owner

minad commented Dec 15, 2020

@ackerleytng I will take a look and make some review soon!

Copy link
Owner

@minad minad left a comment

Choose a reason for hiding this comment

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

@ackerleytng I added some first comments!

@minad
Copy link
Owner

minad commented Dec 15, 2020

@ackerleytng

Seeking comments on code style and implementation as I learn elisp!

I think what you are writing here is already good. I tend to use fewer small functions in elisp since they pollute the namespace/imenu/help-view. And there is not a big benefit in using small functions which are used only once. Furthermore elisp is not as composable as other functional programming languages.

Question: How do you develop on consult? Do you symlink this directory into your ~/.emacs.d?

I usually load/install the package by hand (either via load or via package-install-file) and then execute the sexps in the editor directly while editing. This allows me to adjust functions, test changes, repeat. From time to time reload everything if it is necessary. This works well for me, but maybe there is a better way to do things.

@minad minad marked this pull request as draft December 15, 2020 13:10
@tomfitzhenry
Copy link
Contributor

Per #6 (comment) , consider using xref facilities, xref-search-program, xref-matches-in-files. Emacs already has support for searching using grep or ag. It'd be less code to maintain if that was reused, and the xref to completing-read code could likely be reused for other consult functions in future.

@minad
Copy link
Owner

minad commented Dec 15, 2020

@tomfitzhenry I also thought about that. See also https://github.com/travitch/completing-read-xref.el by @travitch. Does it make sense to have this here?

@ackerleytng Could you investigate the xref facilities a bit? I am personally not very familiar with the xref facilities but I had hoped before to have things more integrated with xref.

@manuel-uberti
Copy link

@tomfitzhenry I also thought about that. See also https://github.com/travitch/completing-read-xref.el by @travitch. Does it make sense to have this here?

FTR, in Emacs 28 one could set xref-show-definitions-function to #'xref--show-defs-minibuffer to have completing-read available.

@minad
Copy link
Owner

minad commented Dec 15, 2020

@manuel-uberti Thanks! I guess it makes only sense to have the command here if it provides something on top of the standard xref function, e.g., if we have preview or some other advantages.

@tomfitzhenry
Copy link
Contributor

tomfitzhenry commented Dec 15, 2020

FTR, in Emacs 28 one could set xref-show-definitions-function to #'xref--show-defs-minibuffer to have completing-read available.

You can do this in Emacs 27. It just requires xref >= 1.0.4 from https://elpa.gnu.org/packages/xref.html

(setq xref-search-program 'ripgrep)
(setq xref-show-xrefs-function 'xref--show-defs-minibuffer)

Then (project-find-regexp) (and typing "defsubst" into the prompt) looks like:

image

@minad
Copy link
Owner

minad commented Dec 15, 2020

@tomfitzhenry what do you suggest? Does it still make sense to provide this here with preview support? The implementation could be taken/inspired by the project.el implementation. Another idea - we could try to modify the preview support such that it can enhance already existing commands. But I am not sure how easy this will be and to which commands it would apply.

@tomfitzhenry
Copy link
Contributor

tomfitzhenry commented Dec 15, 2020

@tomfitzhenry what do you suggest? Does it still make sense to provide this here with preview support?

I think if consult chooses to implement this, it should only be to add on what isn't provided by xref itself. Perhaps that is preview support (which isn't a strong factor for me, but might be for others). If so, it should do so by implementing xref-show-xrefs-function, and then document how to use this, and possibly provide an interactive function to use it:

(defun consult-project-ripgrep ()
  (interactive)
  (let ((xref-search-program 'ripgrep)
        (xref-show-xrefs-function 'consult-xref-show-xrefs-function))
    (call-interactively 'project-find-regexp)))

I don't think we want to be in the space of customising search programs, or parsing their output.

@ackerleytng
Copy link
Author

I think @tomfitzhenry 's suggestion pretty much solves it for me in terms of search and filtering, that output is also nicely colored, and also jumps to the column!

Now for one more request - how would I implement something like wgrep-ag where, after showing all the search results in the minibuffer, I can do C-x C-e to bring up another buffer to edit all the results and then commit them back to multiple files? https://github.com/mhayashi1120/Emacs-wgrep

@manuel-uberti
Copy link

@ackerleytng Do you mean you have an implementation for consult-xref-show-xrefs-function?

@manuel-uberti
Copy link

Now for one more request - how would I implement something like wgrep-ag where, after showing all the search results in the minibuffer, I can do C-x C-e to bring up another buffer to edit all the results and then commit them back to multiple files? https://github.com/mhayashi1120/Emacs-wgrep

Something similar is already provided I guess: xref-query-replace-in-results.

@manuel-uberti
Copy link

@ackerleytng Do you mean you have an implementation for consult-xref-show-xrefs-function?

Actually, setting xref-show-xrefs-function to #'xref--show-defs-minibuffer should be enough. @tomfitzhenry what do you think?

@ackerleytng
Copy link
Author

@ackerleytng Do you mean you have an implementation for consult-xref-show-xrefs-function?

Nope I meant that xref--show-defs-minibuffer is good enough.

Also, xref-query-replace-in-results doesn't provide the interactivity that wgrep-ag does. With wgrep-ag, you can filter first, so that the result set is trimmed, then edit only those.

@s-kostyaev
Copy link

@tomfitzhenry what do you suggest? Does it still make sense to provide this here with preview support?

I think if consult chooses to implement this, it should only be to add on what isn't provided by xref itself. Perhaps that is preview support (which isn't a strong factor for me, but might be for others). If so, it should do so by implementing xref-show-xrefs-function, and then document how to use this, and possibly provide an interactive function to use it:

(defun consult-project-ripgrep ()
  (interactive)
  (let ((xref-search-program 'ripgrep)
        (xref-show-xrefs-function 'consult-xref-show-xrefs-function))
    (call-interactively 'project-find-regexp)))

I don't think we want to be in the space of customising search programs, or parsing their output.

Perfect. Would be nice if consult preview also will works here.

@minad
Copy link
Owner

minad commented Dec 19, 2020

What is the status here? How should we proceed here? I guess it would still be nice to have a consult variant of project-find-regexp with preview. Furthermore we may be able to add the dynamic aspect such that the search already starts after having entered a few keys.

@ackerleytng
Copy link
Author

I'm trying to get a feel of whether I like the experience of being forced to decide what i want to search for in a separate step from the filtering. If the experience from helm-projectile-ag (search and filter, interleaved) is preferred, then I might want to invest more there. For now seeing the preview of the line itself seems sufficient to me...

@minad
Copy link
Owner

minad commented Dec 24, 2020

I pushed a first version of consult-show-xrefs here - https://github.com/minad/consult/blob/xref/consult-xref.el

@minad minad mentioned this pull request Dec 24, 2020
20 tasks
@manuel-uberti
Copy link

manuel-uberti commented Dec 24, 2020

Thanks for that. I am using it like this:

(defun consult-project-rg ()
    (interactive)
    (let ((xref-show-xrefs-function #'consult-show-xrefs))
      (call-interactively 'project-find-regexp)))

But after hitting RET at the Find regexp prompt to accept the thing at point as the thing to search for, I get this:

Debugger entered--Lisp error: (error "xref-bogus-location-message is already defined as ...")
  signal(error ("xref-bogus-location-message is already defined as ..."))
  error("%s is already defined as something else than a gen..." xref-bogus-location-message)
  cl-generic-ensure-function(xref-bogus-location-message)
  cl-generic-define-method(xref-bogus-location-message nil ((this xref-bogus-location)) nil #f(compiled-function (this) "Access the slot `message' from object of class `xref-bogus-location'." #<bytecode 0x8d9a5b983da1077>))
  byte-code("\300\301\302\303\302\304%\210\300\305\302\306\302\307%\210\310\311\312\313!\"\210\310\314\315\313!\"\210\310\316\314\"\210\317\316\320\321#\210\322\313\323\314#\324\313\325\326..." [cl-generic-define-method xref-location-marker nil ((l xref-buffer-location)) #f(compiled-function (l) #<bytecode -0xdab6ade345e11b2>) xref-location-group ((l xref-buffer-location)) #f(compiled-function (l) #<bytecode 0x13149baa216a0d69>) defalias xref-bogus-location-p eieio-make-class-predicate xref-bogus-location xref-bogus-location--eieio-childp eieio-make-child-predicate xref-bogus-location-child-p make-obsolete "use (cl-typep ... \\='xref-bogus-location) instead" "25.1" define-symbol-prop cl-deftype-satisfies eieio-defclass-internal (xref-location) ((message :type string :initarg :message :reader xref-bogus-location-message)) (:documentation "Bogus locations are sometimes useful to\nindicate e...") xref-bogus-location-message ((this xref-bogus-location)) #f(compiled-function (this) "Access the slot `message' from object of class `xref-bogus-location'." #<bytecode 0x8d9a5b983da1077>)] 7)
  require(xref)
  project-find-regexp(#("xref-pop-to-location" 0 20 (fontified t)))
  funcall-interactively(project-find-regexp #("xref-pop-to-location" 0 20 (fontified t)))
  call-interactively(project-find-regexp)
  (let ((xref-show-xrefs-function #'consult-show-xrefs)) (call-interactively 'project-find-regexp))
  consult-project-rg()
  funcall-interactively(consult-project-rg)
  call-interactively(consult-project-rg nil nil)
  command-execute(consult-project-rg)

@minad
Copy link
Owner

minad commented Dec 24, 2020

@manuel-uberti It works for me. I cannot help debugging this right now - it is just a poc, maybe you can figure it out. I am not sure if I want to pursue the route via xref. Xref is only synchronous and to me it feels over-engineered for what it offers (I guess, I am not a fan of CLOS). Right now I am looking into the possibility to make the search asynchronous such that grep/rg must not have finished for the candidate selection to get started - see radian-software/selectrum#306. It would still be nice to integrate somehow via xref. What do you think?

@manuel-uberti
Copy link

Asynchronous search sounds interesting, although for my use cases ripgrep is already fast enough I am happy to let it finish.

About my problem with your solution: I tested it with standard Emacs minibuffer completion, no Selectrum or Icomplete involved.

@minad
Copy link
Owner

minad commented Dec 24, 2020

About my problem with your solution: I tested it with standard Emacs minibuffer completion, no Selectrum or Icomplete involved.

Which solution - the asynchronous one? I think this can also work with default completion.

@manuel-uberti
Copy link

I meant consult-show-xrefs.

@minad
Copy link
Owner

minad commented Dec 25, 2020

See here for an updated version of the asynchronous proposal #91. The downside is that we cannot use anything from the xref infrastructure, but parsing grep/rg output is not hard. We could still generate xref-items if that is desired and in order to allow integration with xref facilities. Alternatively consult-show-xrefs could be offered for people who prefer a synchronous search with preview.

@minad
Copy link
Owner

minad commented Dec 27, 2020

I am closing this PR in favor of #91. There exist consult-grep, consult-ripgrep and consult-git-grep prototypes which allow to collect the candidates asynchronously. I think it still needs some work until it can be considered "stable", but I think it is on a good way. The asynchronous approach works with selectrum, icomplete, default completion and embark-occur/live-occur. Right now there is no support yet for project roots and no support yet to only search a list of files. It would be nice to hear some feedback regarding the consult-grep commands. What are your expectations regarding such a command?

Independent of #91 there exists the xref branch with consult-show-xrefs (see https://github.com/minad/consult/blob/xref/consult-xref.el). This could be pursued independently.

Ping @ackerleytng @manuel-uberti @tomfitzhenry @s-kostyaev

@minad minad closed this Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants