Package: elpa;
Reported by: Stefan Monnier <monnier <at> iro.umontreal.ca>
Date: Sat, 3 Feb 2024 22:32:01 UTC
Severity: normal
View this message in rfc822 format
From: Rahguzar <rahguzar <at> zohomail.eu> To: Philip Kaludercic <philipk <at> posteo.net> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 68915 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com> Subject: bug#68915: Request to include a couple of packages in GNU ELPA Date: Sun, 04 Feb 2024 20:48:19 +0100
Hi Philip,
Thanks a lot for the comments. I will make some code change but this
email is to discuss some of the points:
Philip Kaludercic <philipk <at> posteo.net> writes:
> @@ -26,17 +27,17 @@
> (defcustom consult-hoogle-args
> '("hoogle" . ("search" "--jsonl" "-q" "--count=250"))
> "The hoogle invocation used to get results.
> -It is should be a cons (COMMAND . ARGS). COMMAND should be valid executable.
> +It is should be a cons (COMMAND . ARGS). COMMAND should be valid executable.
> It is called arguments ARGS with the search query appended. It should produce
> search results in JSON lines format."
> :type '(cons (string :tag "Hoogle command")
> (repeat :tag "Args for hoogle" string))
> - :group 'consult)
> + :group 'consult) ;wouldn't it be better to define your own group?
I can define a new group but there are only two custom variables. I
think this makes sense in light of your comments to reduce coupling with
consult (which I agree with) so I will do it. OTOH I am bit hesitant to
proliferate too many groups with not many options.
> @@ -52,6 +53,9 @@ default it uses `cabal-hoogle' https://github.com/kokobd/cabal-hoogle ."
> (defvar consult-hoogle-map
> (let ((map (make-sparse-keymap)))
> + ;; if this is building on consult, can you rebind some keys
> + ;; instead of hard-coding key chords that the user might have
> + ;; modified?
> (define-key map (kbd "M-i") #'consult-hoogle-browse-item)
> (define-key map (kbd "M-j") #'consult-hoogle-browse-package)
> (define-key map (kbd "M-m") #'consult-hoogle-browse-module)
I don't know which keys to rebind in this case since these commands have
no general analogue. The map was my means of allowing users to override
the bindings without too much hassle. I would also like to solicit some
suggestions for better bindings. I normally use evil so if you or
someone has suggestions for bindings that mesh better with default ones,
I will gladly change these.
> @@ -80,7 +84,7 @@ default it uses `cabal-hoogle' https://github.com/kokobd/cabal-hoogle ."
> "Fontify TEXT, returning the fontified text.
> This is adapted from `haskell-fontify-as-mode' but for better performance
> we use the same buffer throughout."
> - (with-current-buffer " *Hoogle Fontification*"
> + (with-current-buffer " *Hoogle Fontification*" ;why not use a temporary buffer?
'haskell-mode' can be slow and with preview and vertical completion UIs
the temporary buffer will be created many many times. I started with
that and changed to this approach since there were noticeable pauses.
> (erase-buffer)
> (insert text)
> (font-lock-ensure)
> @@ -217,7 +221,7 @@ STATE is the optional state function passed to the `consult--read'."
> (let ((consult-async-min-input 0)
> (fun (or action (lambda (alist) (consult-hoogle--browse-url 'item alist)))))
> (with-current-buffer (get-buffer-create " *Hoogle Fontification*" t)
> - (let (haskell-mode-hook)
> + (let (haskell-mode-hook) ;do you always want this?
I think so, real haskell-mode buffer can have a lot of hooks (like lsp
related ones). OTOH some of them might affect fontification. I think a
separate hook containing a whitelist makes sense but I would rather wait
for someone to complain before adding it.
> (haskell-mode)))
> (unwind-protect
> (funcall fun (consult--read
> @@ -242,11 +246,11 @@ STATE is the optional state function passed to the `consult--read'."
> (defun consult-hoogle (arg)
> "Search the local hoogle database.
> By default this shows the documentation for the current candidate in a side
> -window. This can be disabled by a prefix ARG."
> +window. This can be disabled by a prefix ARG."
> (interactive (list current-prefix-arg))
> (if arg (consult-hoogle--search)
> (let* ((buf (get-buffer-create " *Hoogle Documentation*" t))
> - (height (if (bound-and-true-p vertico-count)
> + (height (if (bound-and-true-p vertico-count) ;can this support icomplete as well?
This is indeed hack and I am more inclined to remove it and make the
height a custom variable. Another route would be to make the display
action a custom variable. What do you think?
> vertico-count
> 10))
> (window (display-buffer buf
> @@ -267,7 +271,7 @@ By default uses cabal-hoogle and the database should have been generated
> by running `cabal-hoogle generate'. `consult-hoogle-project-args' can be
> customized to configure an alternate command.
> By default this shows the documentation for the current candidate in a side
> -window. This can be disabled by a prefix ARG."
> +window. This can be disabled by a prefix ARG."
> (interactive (list current-prefix-arg))
> (let ((consult-hoogle-args consult-hoogle-project-args)
> (default-directory (haskell-cabal-find-dir)))
> @@ -300,13 +304,15 @@ window. This can be disabled by a prefix ARG."
> (scroll-up arg)))
>
> (defun consult-hoogle-restrict-to-package (package &optional arg)
> - "Restrict the search to PACKAGE. With prefix ARG exluce package from search."
> + "Restrict the search to PACKAGE.
> +With prefix ARG exluce package from search."
> (interactive (list (consult-hoogle--get 'package) current-prefix-arg))
> (when package
> (consult-hoogle--add-to-input (if arg "-" "+") (downcase package))))
>
> (defun consult-hoogle-restrict-to-module (module &optional arg)
> - "Restrict the search to MODULE. With prefix ARG exluce module from search."
> + "Restrict the search to MODULE.
> +With prefix ARG exluce module from search."
> (interactive (list (consult-hoogle--get 'module) current-prefix-arg))
> (when module (consult-hoogle--add-to-input (if arg "-" "+") module)))
>
> @@ -319,8 +325,7 @@ If called with numeric prefix LEVEL only use first ARG levels of module."
> (consult-hoogle--add-to-input
> (if (> level 0) "+" "-")
> (progn
> - (string-match (rx-to-string
> - `(: bos (= ,(abs level) (: (1+ (not ".")) (?? ".")))))
> + (string-match (rx bos (= ,(literal (abs level)) (: (1+ (not ".")) (?? "."))))
> module)
I think this is not a valid rx form. Literal can only be a literal
string to match and in (= N rx) N can only be a literal positive integer.
> (match-string 0 module)))))
>
> @@ -333,22 +338,21 @@ If called with numeric prefix LEVEL only use last ARG levels of module."
> (consult-hoogle--add-to-input
> (if (> level 0) "+" "-")
> (progn
> - (string-match (rx-to-string
> - `(: (= ,(abs level) (: (1+ (not ".")) (?? "."))) eos))
> + (string-match (rx (= (literal (abs level)) (: (1+ (not ".")) (?? "."))) eos)
> module)
> (match-string 0 module)))))
>
> (defun consult-hoogle-clear-restrictions (arg)
> "Clear all restrictions and exclusions on the search.
> -With positive prefix ARG only clear restrictions. With negative prefix
> +With positive prefix ARG only clear restrictions. With negative prefix
> only clear exclusions."
> (interactive (list (when current-prefix-arg
> (prefix-numeric-value current-prefix-arg))))
> - (let* ((restriction-rx (rx-to-string `(: ,(if (not arg)
> - '(or "+" "-")
> - (if (> arg 0) "+" "-"))
> - (0+ (not space))
> - (or (1+ space) eos)))))
> + (let* ((restriction-rx (rx
> + (literal (if (not arg)
> + '(or "+" "-")
> + (if (> arg 0) "+" "-")))
> + (0+ (not space)))))
Here too I think literal in rx can't handle arbitrary lisp expressions.
> (consult-hoogle--modify-async-input
> (lambda (match) (replace-regexp-in-string restriction-rx "" match)))))
>
>
>
> Also, it would be nice to have a .elpaignore file in your repository to
> avoid bundling the bitmap images from your repository with the tarballs.
I will look up the format and add the file.
> Generally speaking, how tightly integrated is this into consult, and can
> you imaging generalising this for all users? Having a dependency on a
> UI component seems like a backwards thing to do.
I think it is not too tightly integrated. If there is another easy
way to define an async completion table (i.e. candidates can arrive in
batches) adapting to it would be fairly easy. However there is no UI
independent way of doing this that I know. In this case I wouldn't call
consult a UI component, it rather provides means for me not to deal with
different UI's directly. If you have suggestions in this area, I can
try.
Previews are also provided by a consult protocol but adapting them to a
different protocol wouldn't be too difficult.
These two aspects only concern a small amount of code but they are very
important for a good experience.
Rahguzar
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.