Package: elpa;
Reported by: Stefan Monnier <monnier <at> iro.umontreal.ca>
Date: Sat, 3 Feb 2024 22:32:01 UTC
Severity: normal
Message #35 received at 68915 <at> debbugs.gnu.org (full text, mbox):
From: Philip Kaludercic <philipk <at> posteo.net> To: Rahguzar <rahguzar <at> zohomail.eu> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 68915 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com> Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA Date: Mon, 05 Feb 2024 19:29:05 +0000
Rahguzar <rahguzar <at> zohomail.eu> 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 <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.
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.