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 consult--find accept an "initial" argument? #144

Closed
protesilaos opened this issue Jan 7, 2021 · 15 comments
Closed

Make consult--find accept an "initial" argument? #144

protesilaos opened this issue Jan 7, 2021 · 15 comments

Comments

@protesilaos
Copy link
Sponsor

Hello Daniel! I just noticed that the argument list for consult--find differs from that of consult--grep. Maybe there is some good reason for it. Do you think, however, that it would be possible to at least accept an initial arg? There are a few cases that are affected, but this prototype worked in my quick demo:

 consult.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/consult.el b/consult.el
index 54e9b16..c55ad0b 100644
--- a/consult.el
+++ b/consult.el
@@ -2316,7 +2316,7 @@ CMD is the find argument list."
     (consult--async-throttle)
     (consult--async-split)))
 
-(defun consult--find (prompt cmd)
+(defun consult--find (prompt cmd initial)
   "Generalized function for `consult-find' and similar functions.
 
 PROMPT is the prompt.
@@ -2327,7 +2327,7 @@ CMD is the find argument list."
     (consult--find-async cmd)
     :sort nil
     :require-match t
-    :initial consult-async-default-split
+    :initial (concat consult-async-default-split initial)
     :add-history (concat consult-async-default-split (thing-at-point 'symbol))
     :category 'file
     :history '(:input consult--find-history))))
@minad minad closed this as completed in 49c959c Jan 7, 2021
@minad
Copy link
Owner

minad commented Jan 7, 2021

Hi Prot,

thank you for reporting this! This is an oversight of mine when the initial argument has been added to grep. Note that there is also the option to have fine-grained control over the arguments passed to consult--read on a per-command basis, see the note in the end of https://github.com/minad/consult#customizable-variables.

Daniel

@minad
Copy link
Owner

minad commented Jan 7, 2021

I added an extra section for the consult--read override, see https://github.com/minad/consult#configuration-of-individual-commands. Since you also mentioned the documentation in your video and you praised the already existing quality, I would like to point out that the documentation as of now is still rather minimal and needs further improvements. In particular, I would very much like to improve the self-documentation. See also #99, #124 and #128. Please let me know if you find things which are unclear or need immediate fixing or if you have other ideas!

@protesilaos
Copy link
Sponsor Author

minad closed this in 49c959c 26 minutes ago

Thanks for the quick fix!

Note that there is also the option to have fine-grained control over the arguments passed to consult--read on a per-command basis [...]

I added an extra section for the consult--read override [...]

That is promising!

Since you also mentioned the documentation in your video and you praised the already existing quality, I would like to point out that the documentation as of now is still rather minimal and needs further improvements.

I think it is of a high standard already, though I agree that it can always be improved (and I am still reading through the code base when I find time for it). The doc strings could benefit from some cross-references or more examples. Such as:

 consult.el | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/consult.el b/consult.el
index 54e9b16..94f1a9b 100644
--- a/consult.el
+++ b/consult.el
@@ -64,7 +64,10 @@
 
 ;;;###autoload
 (define-minor-mode consult-preview-mode
-  "Enable preview for consult commands."
+  "Enable preview for all (some?) consult commands.
+
+Preview can be disabled on a per-command basis by configuring
+`consult-config-alist'.
+
+(if 'some', then provide a list somewhere)"
   :global t)
 
 (defcustom consult-narrow-key nil
@@ -209,8 +212,18 @@ You may want to add a function which pulses the current line, e.g.,
   "Number of files to keep open at once during preview."
   :type 'integer)
 
-(defcustom consult-config nil
-  "Command configuration alists."
+(define-obsolete-variable-alias
+  'consult-config
+  'consult-config-alist
+  "VERSION HERE")
+
+(defcustom consult-config-alist nil
+  "Alist of configurations for individual commands.
+
+Each list consists of a command's symbol and a plist, such
+as: (consult-theme :preview-key nil).
+
+Available plist entries can in found in WHERE?."
   :type '(list (cons symbol plist)))
 
 ;;;; Faces

P.S. consult-config-alist felt more natural, so I thought I would mention it now that I saw it.

@minad
Copy link
Owner

minad commented Jan 7, 2021

I think it is of a high standard already, though I agree that it can always be improved (and I am still reading through the code base when I find time for it). The doc strings could benefit from some cross-references or more examples. Such as:

Cross references are a good idea!

consult-config-alist felt more natural, so I thought I would mention it now that I saw it.

I am not sure about this naming convention - I think I've seen both ways and I am usually not a big fan of type information in names. But for lists it is quite common and maybe more readable here.

The deprecation info you proposed is a good idea in general but for now I just break things, I will stop at some point when things have stabilized more. The consult-config feature has been introduced a few days ago, so I can just break it now.

At some point I should probably tag a release and from that point on a stronger stability guarantee should hold.

@minad
Copy link
Owner

minad commented Jan 7, 2021

Regarding consult-config - I checked this again and the issue is that I don't use the -alist or -list suffix for any of the other customizable variables and I don't want to rename them all. Maybe consult-config is too generic and it should be called something like consult-fine-config, consult-command-config, consult-read-config, consult-detail-config, I don't know. Do you have a better name?

@protesilaos
Copy link
Sponsor Author

or any of the other customizable variables and I don't want to rename them all.

You are right. Don't do that.

Maybe consult-config is too generic and it should be called something like [...] Do you have a better name?

consult-command-config sounds good.

@minad
Copy link
Owner

minad commented Jan 7, 2021

consult-command-config - there is also a problem with this one: the name command is also too generic, since we have consult commands like consult-mode-command and consult-complex-command. I think it should be even more specific such that it is not misleading, consult-read-config is my favorite now, but it unnecessarily exposes the name read, which is still private API and I guess it will stay so. Another option could be consult-completion-config. But this creates associations with the underlying completion system. I really don't know 😄

@minad
Copy link
Owner

minad commented Jan 7, 2021

Btw @protesilaos, I think I recall correctly that you are one of the people, who do not prefer to use automatic preview since it makes things jump around. Maybe this is interesting for you #143.

@protesilaos
Copy link
Sponsor Author

Very well! Will pull the changes later and adapt things accordingly.

@protesilaos
Copy link
Sponsor Author

consult-command-config - there is also a problem with this one: the name command is also too generic, since we have consult commands like consult-mode-command and consult-complex-command. I think it should be even more specific such that it is not misleading, consult-read-config is my favorite now, but it unnecessarily exposes the name read, which is still private API and I guess it will stay so. Another option could be consult-completion-config. But this creates associations with the underlying completion system. I really don't know smile

Oh, I see! How about something like consult-command-{parameters,controls}? I know you prefer singular, so please adapt accordingly.

@protesilaos
Copy link
Sponsor Author

I am now trying this and it also works with my quick-and-dirty wrapper functions (those that preconfigure orderless):

  (setq consult-preview-key nil) ; see `consult-config' further below

  (let ((key (kbd "C-v"))) ; "view" mnemonic
    (setq consult-config `((consult-buffer :preview-key ,key)
                           (consult-line :preview-key ,key)
                           (prot-consult-line :preview-key ,key))))

Setting consult-preview-key to "C-v" did not seem to work, though I may have done something wrong...

@minad
Copy link
Owner

minad commented Jan 7, 2021

I would write it like this:

(setq consult-config '((consult-buffer :preview-key "\C-v")
                           (consult-line :preview-key "\C-v")
                           (prot-consult-line :preview-key "\C-v"))))

For C-v you can use a normal character, since it is an ASCII control key.

This works also, and just enables the key for every command. Note that preview is generally very benign, no open files will be left behind etc. So you can enable if for everything.

(setq consult-preview-key "\C-v")

@protesilaos
Copy link
Sponsor Author

(setq consult-preview-key "\C-v")

This is what I did wrong. I used "C-v". Thanks!

@minad
Copy link
Owner

minad commented Jan 7, 2021

Yes, I am not passing everything through kbd as of now. I expect the low-level format. Maybe I should change this. But consult is in the end just aligned with define-key.

@protesilaos
Copy link
Sponsor Author

Yes, I am not passing everything through kbd as of now. I expect the low-level format. Maybe I should change this. But consult is in the end just aligned with define-key.

I see. No problem! I always forget when to prepend the backslash, so I use kbd when I have to. Even with define-key:

(define-key map (kbd "C-v") command)

Though I normally just test the key with C-h k and quote what it gives me.

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

No branches or pull requests

2 participants