Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA

From: Philip Kaludercic <[email-masked]>
Date: Mon, 05 Feb 2024 19:29:05 +0000

Rahguzar <[email-masked]> writes:

> 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 <[email-masked]> 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.

I wouldn't worry too much about that, being able to easily and
predictably display all the options that a package or feature provides
is IMO preferable, considering that the number of groups is usually
nothing users have to concern themselves with too much.

>> @@ -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.

I assumed that you were overriding keys that consult or some other
framework was binding, if that is not the case, then nevermind this
comment.

>> @@ -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.

OK, then that is worth adding as a comment.

>>      (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.

1+

>>          (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?

I cannot comment, since I don't understand the intended effect well enough.

>>                         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.

Whoops, you are right, the comma is enough to disqualify it from being a
valid form.

>>         (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.

This expands to

(concat
 (regexp-quote (if (not arg) '(or "+" "-") (if (> arg 0) "+" "-")))
 "[^[:space:]]*")

so the issue is that one of the cases results in (or "+" "-") that
cannot be passed to `regexp-quote'.  If instead we wrote

(rx
 (regexp (cond
	  ((not arg) "[+-]")
	  ((> arg 0) "\\+")
	  (t "-")))
 (0+ (not space)))

then it should work.

>
>>      (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.

The file format is basically just the same as what tar accepts with the
-X argument.

>> 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.

Again, I cannot comment on it, I just want to raise the point that by
depending on an external package like this, you are restricting who will
be interested in the package.  I for one would much prefer to not have
the results displayed asynchronously, but in a fixed buffer that I can
operate using all the usual commands one operates any text buffer.

> Rahguzar