GNU bug report logs - #68915
Request to include a couple of packages in GNU ELPA

Previous Next

To reply to this bug, email your comments to 68915 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: submit <at> debbugs.gnu.org
Subject: Request to include a couple of packages in GNU ELPA
Date: Sat, 03 Feb 2024 17:31:29 -0500

Package:elpa

Stefan Kangas [2024-02-03 17:11:28] wrote:

> Rahguzar <rahguzar <at> zohomail.eu> writes:
>
>> Dear Emacs developers,
>>
>> I would like to propose a couple of my packages for inclusion in GNU
>> ELPA. They are:
>>
>> 1) consult-hoogle: https://codeberg.org/rahguzar/consult-hoogle
>> It allows the use of hoogle search engine for haskell programming
>> language from Emacs using the interfaces provided by consult package.
>>
>> 2) filechooser.el: https://codeberg.org/rahguzar/filechooser
>> It implements the backend D-bus methods for xdg filechooser and can be
>> used to provide an Emacs based file selection interface for applications
>> such as web browsers which support using xdg desktop portals for this
>> purpose.
>>
>>
>> Please let me know of any suggestions and if I need to change anything.
>>
>> Thanks,
>> Rahguzar
>
> I'm adding the GNU ELPA maintainers to CC.
>
> Thanks for your contributions.

Moving this to Debbugs.

Message #8 received at 68915 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Rahguzar <rahguzar <at> zohomail.eu>, Stefan Kangas <stefankangas <at> gmail.com>,
 68915 <at> debbugs.gnu.org
Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Sun, 04 Feb 2024 14:51:31 +0000

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

> Package:elpa
>
> Stefan Kangas [2024-02-03 17:11:28] wrote:
>
>> Rahguzar <rahguzar <at> zohomail.eu> writes:
>>
>>> Dear Emacs developers,
>>>
>>> I would like to propose a couple of my packages for inclusion in GNU
>>> ELPA. They are:
>>>
>>> 1) consult-hoogle: https://codeberg.org/rahguzar/consult-hoogle
>>> It allows the use of hoogle search engine for haskell programming
>>> language from Emacs using the interfaces provided by consult package.

Here a few comments on this package:

[Message part 2 (text/plain, inline)]

diff --git a/consult-hoogle.el b/consult-hoogle.el
index 6922e5d..855c17c 100644
--- a/consult-hoogle.el
+++ b/consult-hoogle.el
@@ -1,19 +1,20 @@
 ;;; consult-hoogle.el --- Hoogle frontend using consult -*- lexical-binding: t; -*-
-;;
+
 ;; Created: April 10, 2022
-;; Modified: April 10, 2022
+;; Modified: April 10, 2022  <-- remove this if you don't keep it updated
 ;; License: GPL-3.0-or-later
 ;; Version: 0.0.1
 ;; Keywords: docs languages
 ;; Homepage: https://github.com/aikrahguzar/consult-hoogle
 ;; Package-Requires: ((emacs "27.1") (haskell-mode "16.1"))
-;;
+
 ;; This file is not part of GNU Emacs.
-;;
+
 ;;; Commentary:
+
 ;; Search the local hoogle database from Emacs using the nicities provided by
 ;; consult.
-;;
+
 ;;; Code:
 
 ;;;; Packages
@@ -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?
 
 (defcustom consult-hoogle-project-args
   '("cabal-hoogle" . ("run" "--" "search" "--jsonl" "-q" "--count=250"))
   "The cabal-hoogle invocation used to get results for current project.
-It should be cons (COMMAND . ARGS). See `consult-hoogle-args' for details.  By
+It should be cons (COMMAND . ARGS).  See `consult-hoogle-args' for details.  By
 default it uses `cabal-hoogle' https://github.com/kokobd/cabal-hoogle ."
   :type '(cons (string :tag "Project specific hoogle command")
                (repeat :tag "Args for hoogle" string))
@@ -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)
@@ -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?
     (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?
         (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?
                        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)
        (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)))))
     (consult-hoogle--modify-async-input
      (lambda (match) (replace-regexp-in-string restriction-rx "" match)))))
 

[Message part 3 (text/plain, inline)]

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.

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.

-- 
Philip Kaludercic

Message #11 received at 68915 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Rahguzar <rahguzar <at> zohomail.eu>, Stefan Kangas <stefankangas <at> gmail.com>,
 68915 <at> debbugs.gnu.org
Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Sun, 04 Feb 2024 15:11:39 +0000

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>> 2) filechooser.el: https://codeberg.org/rahguzar/filechooser
>>> It implements the backend D-bus methods for xdg filechooser and can be
>>> used to provide an Emacs based file selection interface for applications
>>> such as web browsers which support using xdg desktop portals for this
>>> purpose.

I don't really understand what this package is doing, so my comments are
more superficial, hope they are nevertheless at least a bit useful:

[Message part 2 (text/plain, inline)]

diff --git a/filechooser.el b/filechooser.el
index cc96386..e08f4b1 100644
--- a/filechooser.el
+++ b/filechooser.el
@@ -1,26 +1,27 @@
 ;;; filechooser.el --- An xdg-desktop-portal filechooser -*- lexical-binding: t; -*-
-;;
-;; Copyright (C) 2023 azeem
-;;
+
+;; Copyright (C) 2023 azeem <-- is this you?
+
 ;; Author: rahguzar <rahguzar <at> zohomail.eu>
 ;; Maintainer: rahguzar <rahguzar <at> zohomail.eu>
 ;; Created: May 20, 2023
-;; Modified: May 20, 2023
+;; Modified: May 20, 2023 <-- either use time-stamp or remove this please
 ;; Version: 0.0.1
 ;; Keywords: convenience files tools unix
 ;; Homepage: https://codeberg.org/rahguzar/filechooser
 ;; Package-Requires: ((emacs "28.1") (compat "29.1"))
-;;
+
 ;; This file is not part of GNU Emacs.
-;;
+
 ;;; Commentary:
-;; An implementation of xdg-desktop-portal filechooser in Emacs. This allows
+
+;; An implementation of xdg-desktop-portal filechooser in Emacs.  This allows
 ;; for choosing files in applications like firefox (with GTK_USE_PORTAL set)
-;; from an Emacs frame. The default is to use `read-file-name' for choosing
+;; from an Emacs frame.  The default is to use `read-file-name' for choosing
 ;; a single file and a pair of Dired buffers for choosing multiple files.
-;;
-;;
+
 ;;; Code:
+
 (require 'compat)
 (require 'dbus)
 (require 'xdg)
@@ -49,25 +50,27 @@
 
 (defvar filechooser-multiple-selection-map
   (let ((map (make-sparse-keymap)))
+    (set-keymap-parent map filechooser-mininuffer-map)
     (define-key map (kbd "M-TAB") #'filechooser-multiple-continue)
     (define-key map (kbd "M-RET") #'filechooser-multiple-finalize-current-selection)
     (define-key map (kbd "M-a") #'filechooser-multiple-select-all)
-    (make-composed-keymap map filechooser-mininuffer-map)))
+    map))
 
 ;;;; Custom variables
-(defgroup filechooser nil
+(defgroup filechooser nil		;this is a duplicate!
   "An xdg-desktop-portal filechooser."
   :link '(url-link :tag "Homepage" "https://codeberg.org/rahguzar/filechooser")
   :group 'files)
 
 (defcustom filechooser-save-existing-files 'uniquify
   "Determines behavior when attempting to save an existing file FILENAME.
-If it is symbol `yes-or-no-p', `yes-or-no-p' is used to confirm if the file
-should be overwritten. If it is the symbol `y-or-n-p', `y-or-n-p' is used to
-prompt. In both cases if a the answer is negative, the file selection is started
-again. If it is the symbol `uniquify', the FILENAME is made unique by appedning
--N to it where N is a positive number. If it is a function, it is called with
-FILENAME and the return value is used as the filename."
+If it is symbol `yes-or-no-p', `yes-or-no-p' is used to confirm if the
+file should be overwritten.  If it is the symbol `y-or-n-p', `y-or-n-p'
+is used to prompt.  In both cases if a the answer is negative, the file
+selection is started again.  If it is the symbol `uniquify', the
+FILENAME is made unique by appedning -N to it where N is a positive
+number.  If it is a function, it is called with FILENAME and the return
+value is used as the filename."
   :type '(choice
           (const :tag "Uniquify" uniquify)
           (const :tag "Prompt Yes/No" yes-or-no-p)
@@ -80,8 +83,8 @@ If it is nil the selected frame is used instead."
   :type 'boolean)
 
 (defcustom filechooser-filters `(("Directories" filechooser-file-directory-p . t)
-                              ("Elisp files" ,(rx ".el" eos))
-                              ("Not dot files" ,(rx bos (not ?.))))
+				 ("Elisp files" ,(rx ".el" eos))
+				 ("Not dot files" ,(rx bos (not ?.))))
   "An alist of (NAME FILTER . BOOL).
 NAME should describe the filter which can either be a regexp
 or else a predicate function which takes a filename as argument.
@@ -89,8 +92,8 @@ If BOOL is non-nil filter is active by default otherwise it is inactive."
   :type '(alist :key-type (string :tag "Name")
                 :value-type
                 (cons :tag "Value:"
-                  (choice :tag "Filter" regexp function)
-                  (boolean :tag "Default"))))
+                      (choice :tag "Filter" regexp function)
+                      (boolean :tag "Default"))))
 
 (defcustom filechooser-choose-file #'filechooser-read-file-name
   "Function used to choose a single file.
@@ -114,7 +117,7 @@ It should have the same calling convention as
   "The key that is used to exit minibuffer to do completion.
 I.e. the key that binds the equivalent of `exit-minibuffer' for the completion
 UI of choice: usually RET."
-  :type 'key)
+  :type 'key)				;this is not available for Emacs 27
 
 ;;;; Others
 (defvar filechooser-current-operation nil
@@ -122,12 +125,12 @@ UI of choice: usually RET."
 
 ;;;; Internal Variables
 (defvar filechooser--filters nil)
-(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t)))
+(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t))) ;do you really want to evaluate a side effect here?
 (defvar filechooser--multiple-selection nil)
 
 ;;; Filters
 (defun filechooser--filters-group-fn (cand transform)
-  "Group function for selecting filters. CAND TRANSFORM."
+  "Group function for selecting filters.  CAND TRANSFORM." ;please document CAND and TRANSFORM
   (if transform
       cand
     (if (cdr (alist-get cand filechooser--filters nil nil #'equal))
@@ -136,9 +139,9 @@ UI of choice: usually RET."
 
 (defun filechooser-file-directory-p (name)
   "Return non-nil if NAME is an existing directory."
-    (file-directory-p (if (derived-mode-p 'dired-mode)
-                          (expand-file-name name (dired-current-directory))
-                        name)))
+  (file-directory-p (if (derived-mode-p 'dired-mode)
+                        (expand-file-name name (dired-current-directory))
+                      name)))
 
 (defun filechooser-toggle-filter (arg)
   "Toggle a filter.
@@ -170,13 +173,12 @@ With prefix ARG toggle multiple filters using `completing-read-multiple'."
         (current (caar (alist-get "current_filter" opts nil nil #'equal)))
         (regex-filters)
         (glob-to-regexp (lambda (cell) (if (eq  0 (car cell))
-                                      `(regexp ,(dired-glob-regexp (nth 1 cell)))
-                                    ""))))
+					   `(regexp ,(dired-glob-regexp (nth 1 cell)))
+					 ""))))
     (unless (alist-get (car current) filters nil nil #'equal)
       (when current (push current filters)))
     (pcase-dolist (`(,name ,globs) filters)
-      (push (list name
-                  (rx-to-string `(or ,@(mapcar glob-to-regexp globs))))
+      (push (list name (mapconcat #'wildcard-to-regexp "\\|" globs))
             regex-filters))
     (when (and current (not (caar (alist-get "directory" opts nil nil #'equal))))
       (cl-callf not (cdr (alist-get (car current) regex-filters nil nil #'equal))))
@@ -187,9 +189,12 @@ With prefix ARG toggle multiple filters using `completing-read-multiple'."
   (lambda (name)
     (catch 'match
       (dolist (filter filters)
-        (when (if (stringp filter)
-                  (string-match filter name)
-                (funcall filter name))
+        (when (cond
+	       ((stringp filter)
+                (string-match filter name)) ;or string-match-p?
+               ((functionp filter)
+		(funcall filter name))
+	       ((error "Unknown filter %S" filter)))
           (throw 'match t))))))
 
 ;;; Utility definitions
@@ -199,18 +204,18 @@ MINIBUFFER is the value of minibuffer frame paramter."
   (declare (indent 1))
   (let ((framevar (make-symbol "frame")))
     `(let ((minibuffer-completing-file-name ,(eq minibuffer 'only)))
-      (if filechooser-use-popup-frame
-         (let ((,framevar (make-frame '((name . ,(if (eq minibuffer 'only)
-                                                     "filechooser-miniframe"
-                                                   "filechooser-frame"))
-                                        (minibuffer . ,minibuffer)))))
-           (unwind-protect
-               (with-demoted-errors "%S"
-                 (with-selected-frame ,framevar
-                   ,@body))
-             (delete-frame ,framevar 'force)))
-       (with-demoted-errors "%S"
-         ,@body)))))
+       (if filechooser-use-popup-frame
+           (let ((,framevar (make-frame '((name . ,(if (eq minibuffer 'only)
+                                                       "filechooser-miniframe"
+                                                     "filechooser-frame"))
+                                          (minibuffer . ,minibuffer)))))
+             (unwind-protect
+		 (with-demoted-errors "%S"
+                   (with-selected-frame ,framevar
+                     ,@body))
+               (delete-frame ,framevar 'force)))
+	 (with-demoted-errors "%S"
+           ,@body)))))
 
 (defun filechooser-abort ()
   "Abort the file selection."
@@ -244,7 +249,7 @@ See Info node `(elisp) Programmed Completion' for STR, PRED and ACTION."
   (pcase action
     ('t (let ((dir (or (file-name-directory str) default-directory)))
           (cl-callf2 mapcar (lambda (sel) (cons (car sel)
-                                           (file-relative-name (car sel) dir)))
+						(file-relative-name (car sel) dir)))
                      filechooser--multiple-selection)
           (append (mapcar #'cdr filechooser--multiple-selection)
                   (completion-file-name-table str pred t))))
@@ -255,12 +260,12 @@ See Info node `(elisp) Programmed Completion' for STR, PRED and ACTION."
 
 (defun filechooser--read-file-name-1 (prompt &optional mustmatch filters dir default)
   "Read a filename with PROMPT and predicate made from FILTERS.
-MUSTMATCH and DIR are as in `read-file-name'. DEFAULT is the default filename.
+MUSTMATCH and DIR are as in `read-file-name'.  DEFAULT is the default filename.
 If MULTIPLE is non-nil `completing-read-multiple' is used."
   (catch 'continue
     (minibuffer-with-setup-hook
         (lambda () (use-local-map (make-composed-keymap filechooser-mininuffer-map
-                                                   (current-local-map)))
+							(current-local-map)))
           (when dir (setq default-directory dir)))
       (read-file-name
        prompt dir default mustmatch nil
@@ -271,25 +276,25 @@ If MULTIPLE is non-nil `completing-read-multiple' is used."
 DIR is the directory to use if a new file name needs to be choosen and FILTERS
 are the filters to use in that case."
   (pcase filechooser-save-existing-files
-        ('uniquify
-         (let ((n 1)
-               (name (file-name-sans-extension filename))
-               (ext (file-name-extension filename t)))
-           (while (file-exists-p (format "%s-%s%s" name n ext))
-             (cl-incf n))
-           (format "%s-%s%s" name n ext)))
-        ((or 'yes-or-no-p 'y-or-n-p)
-         (if (funcall filechooser-save-existing-files
-                      (format "File %s exists. Overwrite?" filename))
-             filename
-           (filechooser--read-file-name "Choose a new file name: "
-                                        nil filters dir
-                                        (file-relative-name filename dir))))
-        (_ (funcall filechooser-save-existing-files filename))))
+    ('uniquify
+     (let ((n 1)
+           (name (file-name-sans-extension filename))
+           (ext (file-name-extension filename t)))
+       (while (file-exists-p (format "%s-%s%s" name n ext))
+         (cl-incf n))
+       (format "%s-%s%s" name n ext)))
+    ((or 'yes-or-no-p 'y-or-n-p)
+     (if (funcall filechooser-save-existing-files
+                  (format "File %s exists. Overwrite?" filename))
+         filename
+       (filechooser--read-file-name "Choose a new file name: "
+                                    nil filters dir
+                                    (file-relative-name filename dir))))
+    (_ (funcall filechooser-save-existing-files filename))))
 
 (defun filechooser--read-file-name (prompt &optional mustmatch filters dir default)
   "Read a filename with PROMPT and predicate made from FILTERS.
-MUSTMATCH and DIR are as in `read-file-name'. DEFAULT is the default filename.
+MUSTMATCH and DIR are as in `read-file-name'.  DEFAULT is the default filename.
 If MULTIPLE is non-nil `completing-read-multiple' is used."
   (setq filechooser--filters (cl-delete-duplicates
                               (append filechooser-filters filters)
@@ -311,12 +316,12 @@ If MULTIPLE is non-nil `completing-read-multiple' is used."
   "Read a file name.
 If `filechooser-use-popup-frame' is non-nil a new minibuffer only popup frame
 is used, othewise the selected frame is used.
-PROMPT is the minibuffer prompt. MUSTMATCH and DIR are as in `read-file-name'.
-FILTERS take the same form as elements of `filechooser-filters'. Only those
+PROMPT is the minibuffer prompt.  MUSTMATCH and DIR are as in `read-file-name'.
+FILTERS take the same form as elements of `filechooser-filters'.  Only those
 files which satisfy one of the active filters from FILTERS or
 `filechooser-filters' are presented for completions."
   (filechooser--maybe-with-new-frame only
-    (filechooser--read-file-name prompt mustmatch filters dir default)))
+				     (filechooser--read-file-name prompt mustmatch filters dir default)))
 
 (defun filechooser-multiple-continue ()
   "Select current file and exit multiple file selection."
@@ -362,8 +367,8 @@ MAP contains additional key bindigs."
             (catch 'continue
               (minibuffer-with-setup-hook
                   (lambda () (use-local-map (make-composed-keymap
-                                        filechooser-multiple-selection-map
-                                        (current-local-map))))
+                                             filechooser-multiple-selection-map
+                                             (current-local-map))))
                 (completing-read prompt #'filechooser--multiple-loop-table
                                  (filechooser--filters-predicate filters) t
                                  (abbreviate-file-name dir)
@@ -374,8 +379,8 @@ MAP contains additional key bindigs."
   "Read multiple file names using `completing-read-multiple'.
 If `filechooser-use-popup-frame' is non-nil a new minibuffer only popup frame
 is used, othewise the selected frame is used.
-PROMPT is the minibuffer prompt. DIR is the directory where selection starts.
-FILTERS take the same form as elements of `filechooser-filters'. Only those
+PROMPT is the minibuffer prompt.  DIR is the directory where selection starts.
+FILTERS take the same form as elements of `filechooser-filters'.  Only those
 files which satisfy one of the active filters from FILTERS or
 `filechooser-filters' are presented for completions."
   (setq filechooser--filters
@@ -386,35 +391,38 @@ files which satisfy one of the active filters from FILTERS or
              (expand-file-name (or dir default-directory))))
   (let (selected filechooser--multiple-selection)
     (filechooser--maybe-with-new-frame only
-      (while (setq dir
-                   (catch 'done
-                     (setq selected (expand-file-name
-                                     (filechooser--multiple-read-file-name
-                                      prompt dir)))
-                     (cl-callf not
-                         (alist-get selected filechooser--multiple-selection
-                                    nil t #'equal))
-                     (when (eq this-command 'filechooser-multiple-continue)
-                       (expand-file-name (file-name-directory selected))))))
-      (nreverse (mapcar #'car filechooser--multiple-selection)))))
+				       ;; perhaps rewrite this to
+				       ;; improve the default
+				       ;; indentation
+				       (while (setq dir
+						    (catch 'done
+						      (setq selected (expand-file-name
+								      (filechooser--multiple-read-file-name
+								       prompt dir)))
+						      (cl-callf not
+							  (alist-get selected filechooser--multiple-selection
+								     nil t #'equal))
+						      (when (eq this-command 'filechooser-multiple-continue)
+							(expand-file-name (file-name-directory selected))))))
+				       (nreverse (mapcar #'car filechooser--multiple-selection)))))
 
 (defun filechooser-save-files (prompt &optional dir files)
   "Read a directory name to save FILES in it.
 If `filechooser-use-popup-frame' is non-nil a new minibuffer only popup frame
-is used, othewise the selected frame is used. PROMPT and DIR are as in
+is used, othewise the selected frame is used.  PROMPT and DIR are as in
 `read-directory-name'."
   (filechooser--maybe-with-new-frame only
-    (when-let ((save-dir (read-directory-name prompt dir))
-               (names nil))
-      (make-directory save-dir t)
-      (catch 'abort
-        (dolist (file files)
-          (push (if (file-exists-p (expand-file-name file save-dir))
-                    (or (filechooser--handle-exisiting-file file save-dir)
-                        (throw 'abort nil))
-                  (expand-file-name file save-dir))
-                names))
-        names))))
+				     (when-let ((save-dir (read-directory-name prompt dir))
+						(names nil))
+				       (make-directory save-dir t)
+				       (catch 'abort
+					 (dolist (file files)
+					   (push (if (file-exists-p (expand-file-name file save-dir))
+						     (or (filechooser--handle-exisiting-file file save-dir)
+							 (throw 'abort nil))
+						   (expand-file-name file save-dir))
+						 names))
+					 names))))
 
 ;;; Dired based selection
 (let (marked unmarked timer)
@@ -449,14 +457,14 @@ is used, othewise the selected frame is used. PROMPT and DIR are as in
 (defun filechooser-dired (&optional dir filters)
   "Select some files using Dired.
 Running this command pops a Dired for directory DIR, and enters a recursive
-editing session. FILTERS are in the format of `filechooser-filters'."
+editing session.  FILTERS are in the format of `filechooser-filters'."
   (let ((overriding-map `((t . ,filechooser-dired-overriding-map)))
         (apply-filters (lambda (_) (when (and (derived-mode-p 'dired-mode)
-                                         (not (eq (current-buffer) (cdr filechooser--selection)))
-                                         (not (memq 'filechooser--dired-jit-filter jit-lock-functions)))
-                                (add-hook 'jit-lock-functions #'filechooser--dired-jit-filter 95 t)
-                                (jit-lock-mode t)
-                                (add-to-invisibility-spec 'filechooser-filter))))
+                                              (not (eq (current-buffer) (cdr filechooser--selection)))
+                                              (not (memq 'filechooser--dired-jit-filter jit-lock-functions)))
+                                     (add-hook 'jit-lock-functions #'filechooser--dired-jit-filter 95 t)
+                                     (jit-lock-mode t)
+                                     (add-to-invisibility-spec 'filechooser-filter))))
         (selection-buffer (progn (setcdr filechooser--selection nil)
                                  (dired-noselect filechooser--selection))))
     (unwind-protect
@@ -528,8 +536,8 @@ editing session. FILTERS are in the format of `filechooser-filters'."
 (defun filechooser-with-dired (_prompt &optional dir filters)
   "Select some files using Dired.
 If `filechooser-use-popup-frame' is non-nil a new frame is used for selection,
-otherwise selected frame is used. DIR is the directory for initial Dired
-buffer. FILTERS are used to restrict selection to a subset of files."
+otherwise selected frame is used.  DIR is the directory for initial Dired
+buffer.  FILTERS are used to restrict selection to a subset of files."
   (filechooser--maybe-with-new-frame t (filechooser-dired dir filters)))
 
 ;;; Method handlers

[Message part 3 (text/plain, inline)]

-- 
Philip Kaludercic

Message #14 received at 68915 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: 68915 <at> debbugs.gnu.org
Cc: Rahguzar <rahguzar <at> zohomail.eu>, Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Sun, 04 Feb 2024 11:24:01 -0500

>>> I would like to propose a couple of my packages for inclusion in GNU
>>> ELPA. They are:
>>>
>>> 1) consult-hoogle: https://codeberg.org/rahguzar/consult-hoogle
>>> It allows the use of hoogle search engine for haskell programming
>>> language from Emacs using the interfaces provided by consult package.
>>>
>>> 2) filechooser.el: https://codeberg.org/rahguzar/filechooser
>>> It implements the backend D-bus methods for xdg filechooser and can be
>>> used to provide an Emacs based file selection interface for applications
>>> such as web browsers which support using xdg desktop portals for this
>>> purpose.

Thanks, added.

>>> Please let me know of any suggestions and if I need to change anything.

For both of those packages, it'd be nice to add a `.gitignore` with
something like:

    *.elc
    
    # ELPA-generated files
    /consult-hoogle-autoloads.el
    /consult-hoogle-pkg.el

For `consult-hoogle` I suggest you add a `.elpaignore` so as to prevent
the large PNG files to make their way into the tarball, since they're so
much larger than the rest of the package and they're not really useful
in `~/.emacs.d/elpa`.

For `filechooser.el`, I see you bumped into the fact that `bytecomp.el`
doesn't keep track of those functions defined within a `let`, so you
refrained from using #' to refer to those functions.

The better solution is to send a patch for `bytecomp.el`, of course, but
in this specific case you could workaround the problem with something
like the (guaranteed 100% untested) patch below.  It's not very pretty,
so I recommend you look into that `bytecomp.el` patch 🙂.


        Stefan


diff --git a/filechooser.el b/filechooser.el
index cc96386112..7df375807c 100644
--- a/filechooser.el
+++ b/filechooser.el
@@ -417,21 +417,23 @@ is used, othewise the selected frame is used. PROMPT and DIR are as in
         names))))
 
 ;;; Dired based selection
-(let (marked unmarked timer)
-  (defun filechooser--adjust-selection-buffer ()
-    (when (buffer-live-p (cdr filechooser--selection))
-      (with-current-buffer (cdr filechooser--selection)
-        (cl-callf cl-set-difference (cdr dired-directory) unmarked :test #'equal)
-        (when marked
-          (cl-callf nreverse (cdr dired-directory))
-          (dolist (file marked)
-            (cl-pushnew file (cdr dired-directory) :test #'equal))
-          (cl-callf nreverse (cdr dired-directory)))
-        (revert-buffer)))
-    (setq marked nil unmarked nil timer nil))
-
-  (defun filechooser--process-changed-marks (beg end length)
-    "Deal with change in mark from BEG to END with LENGTH."
+(defalias 'filechooser--process-changed-marks
+  (let (marked unmarked timer)
+    (lambda (beg end length)
+      "Deal with change in mark from BEG to END with LENGTH."
+      (cl-flet
+          ((adjust-selection-buffer ()
+             (when (buffer-live-p (cdr filechooser--selection))
+               (with-current-buffer (cdr filechooser--selection)
+                 (cl-callf cl-set-difference (cdr dired-directory) unmarked :test #'equal)
+                 (when marked
+                   (cl-callf nreverse (cdr dired-directory))
+                   (dolist (file marked)
+                     (cl-pushnew file (cdr dired-directory) :test #'equal))
+                   (cl-callf nreverse (cdr dired-directory)))
+                 (revert-buffer)))
+             (setq marked nil unmarked nil timer nil)))
+
     (when (and (derived-mode-p 'dired-mode)
                (eq length 0) (eq (1+ beg) end)
                (not (invisible-p (1- (pos-eol)))))
@@ -444,7 +446,7 @@ is used, othewise the selected frame is used. PROMPT and DIR are as in
           (push (dired-get-filename nil t) unmarked))
         (unless timer
           (setq timer (run-with-timer
-                       0.2 nil 'filechooser--adjust-selection-buffer)))))))
+                       0.2 nil #'adjust-selection-buffer)))))))))
 
 (defun filechooser-dired (&optional dir filters)
   "Select some files using Dired.
@@ -474,7 +476,7 @@ editing session. FILTERS are in the format of `filechooser-filters'."
                  (jit-lock-mode t))
                (push overriding-map emulation-mode-map-alists)
                (add-hook 'window-buffer-change-functions apply-filters)
-               (add-hook 'after-change-functions 'filechooser--process-changed-marks)
+               (add-hook 'after-change-functions #'filechooser--process-changed-marks)
                (setq filechooser--filters (append filechooser-filters filters))
                (other-window 1)
                (dired (or dir default-directory))
@@ -484,7 +486,7 @@ editing session. FILTERS are in the format of `filechooser-filters'."
                    (cdr dired-directory))))
       (cl-callf2 delq overriding-map emulation-mode-map-alists)
       (remove-hook 'window-buffer-change-functions apply-filters)
-      (remove-hook 'after-change-functions 'filechooser--process-changed-marks)
+      (remove-hook 'after-change-functions #'filechooser--process-changed-marks)
       (kill-buffer (cdr filechooser--selection))
       (setcdr filechooser--selection nil)
       (dolist (buf (match-buffers `(derived-mode . dired-mode) (frame-parameter nil 'buffer-list)))

Message #17 received at 68915 <at> debbugs.gnu.org (full text, mbox):

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: Re: 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

Message #20 received at 68915 <at> debbugs.gnu.org (full text, mbox):

From: Rahguzar <rahguzar <at> zohomail.eu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 68915 <at> debbugs.gnu.org
Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Sun, 04 Feb 2024 21:36:55 +0100

Hi Stefan

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>>> I would like to propose a couple of my packages for inclusion in GNU
>>>> ELPA. They are:
>>>>
>>>> 1) consult-hoogle: https://codeberg.org/rahguzar/consult-hoogle
>>>> It allows the use of hoogle search engine for haskell programming
>>>> language from Emacs using the interfaces provided by consult package.
>>>>
>>>> 2) filechooser.el: https://codeberg.org/rahguzar/filechooser
>>>> It implements the backend D-bus methods for xdg filechooser and can be
>>>> used to provide an Emacs based file selection interface for applications
>>>> such as web browsers which support using xdg desktop portals for this
>>>> purpose.
>
> Thanks, added.

Thanks a lot! I think I need to change the version header before they
appear on ELPA? I will deal with feedback here before doing that.

> For both of those packages, it'd be nice to add a `.gitignore` with
> something like:
>
>     *.elc
>
>     # ELPA-generated files
>     /consult-hoogle-autoloads.el
>     /consult-hoogle-pkg.el
>
> For `consult-hoogle` I suggest you add a `.elpaignore` so as to prevent
> the large PNG files to make their way into the tarball, since they're so
> much larger than the rest of the package and they're not really useful
> in `~/.emacs.d/elpa`.

I will add the `.elpaignore` file.

> For `filechooser.el`, I see you bumped into the fact that `bytecomp.el`
> doesn't keep track of those functions defined within a `let`, so you
> refrained from using #' to refer to those functions.

Yes, I find that style quite nice and defvars for localized state feel
wrong but I have mostly come around to that and stopped using top level
let. Maybe a variable for that state is the easiest thing to do.

> The better solution is to send a patch for `bytecomp.el`, of course, but
> in this specific case you could workaround the problem with something
> like the (guaranteed 100% untested) patch below.  It's not very pretty,
> so I recommend you look into that `bytecomp.el` patch 🙂.

That will probably require some CS education :) I am not a coder by
trade rather an aspiring academic in another field.

>         Stefan
>

Message #23 received at 68915 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Sun, 04 Feb 2024 21:57:44 +0100

Hi Philip,
  Thanks again for these.

I will make most of the stylistic changes but I would like to not mix
tabs and spaces for the code I want to maintain. Other than that some
responses to your questions are below:

Philip Kaludercic <philipk <at> posteo.net> writes:

> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>>> 2) filechooser.el: https://codeberg.org/rahguzar/filechooser
>>>> It implements the backend D-bus methods for xdg filechooser and can be
>>>> used to provide an Emacs based file selection interface for applications
>>>> such as web browsers which support using xdg desktop portals for this
>>>> purpose.
>
> I don't really understand what this package is doing, so my comments are
> more superficial, hope they are nevertheless at least a bit useful:
>
> diff --git a/filechooser.el b/filechooser.el
> index cc96386..e08f4b1 100644
> --- a/filechooser.el
> +++ b/filechooser.el
> @@ -1,26 +1,27 @@
>  ;;; filechooser.el --- An xdg-desktop-portal filechooser -*- lexical-binding: t; -*-
> -;;
> -;; Copyright (C) 2023 azeem
> -;;
> +
> +;; Copyright (C) 2023 azeem <-- is this you?
> +

Yes, sorry will change that.

>  ;; Author: rahguzar <rahguzar <at> zohomail.eu>
>  ;; Maintainer: rahguzar <rahguzar <at> zohomail.eu>
>  ;; Created: May 20, 2023
> -;; Modified: May 20, 2023
> +;; Modified: May 20, 2023 <-- either use time-stamp or remove this please
>  ;; Version: 0.0.1
>  ;; Keywords: convenience files tools unix
>  ;; Homepage: https://codeberg.org/rahguzar/filechooser
>  ;; Package-Requires: ((emacs "28.1") (compat "29.1"))
> -;;
> +
>  ;; This file is not part of GNU Emacs.
> -;;
> +
>  ;;; Commentary:
> -;; An implementation of xdg-desktop-portal filechooser in Emacs. This allows
> +
> +;; An implementation of xdg-desktop-portal filechooser in Emacs.  This allows
>  ;; for choosing files in applications like firefox (with GTK_USE_PORTAL set)
> -;; from an Emacs frame. The default is to use `read-file-name' for choosing
> +;; from an Emacs frame.  The default is to use `read-file-name' for choosing
>  ;; a single file and a pair of Dired buffers for choosing multiple files.
> -;;
> -;;
> +
>  ;;; Code:
> +
>  (require 'compat)
>  (require 'dbus)
>  (require 'xdg)
> @@ -49,25 +50,27 @@
>
>  (defvar filechooser-multiple-selection-map
>    (let ((map (make-sparse-keymap)))
> +    (set-keymap-parent map filechooser-mininuffer-map)
>      (define-key map (kbd "M-TAB") #'filechooser-multiple-continue)
>      (define-key map (kbd "M-RET") #'filechooser-multiple-finalize-current-selection)
>      (define-key map (kbd "M-a") #'filechooser-multiple-select-all)
> -    (make-composed-keymap map filechooser-mininuffer-map)))
> +    map))
>
>  ;;;; Custom variables
> -(defgroup filechooser nil
> +(defgroup filechooser nil		;this is a duplicate!

Is the other definition in some package? I didn't see any filechooser
group is options for `customize-group` without this package loaded.

>    "An xdg-desktop-portal filechooser."
>    :link '(url-link :tag "Homepage" "https://codeberg.org/rahguzar/filechooser")
>    :group 'files)
>
>  (defcustom filechooser-save-existing-files 'uniquify
>    "Determines behavior when attempting to save an existing file FILENAME.
> -If it is symbol `yes-or-no-p', `yes-or-no-p' is used to confirm if the file
> -should be overwritten. If it is the symbol `y-or-n-p', `y-or-n-p' is used to
> -prompt. In both cases if a the answer is negative, the file selection is started
> -again. If it is the symbol `uniquify', the FILENAME is made unique by appedning
> --N to it where N is a positive number. If it is a function, it is called with
> -FILENAME and the return value is used as the filename."
> +If it is symbol `yes-or-no-p', `yes-or-no-p' is used to confirm if the
> +file should be overwritten.  If it is the symbol `y-or-n-p', `y-or-n-p'
> +is used to prompt.  In both cases if a the answer is negative, the file
> +selection is started again.  If it is the symbol `uniquify', the
> +FILENAME is made unique by appedning -N to it where N is a positive
> +number.  If it is a function, it is called with FILENAME and the return
> +value is used as the filename."
>    :type '(choice
>            (const :tag "Uniquify" uniquify)
>            (const :tag "Prompt Yes/No" yes-or-no-p)
> @@ -80,8 +83,8 @@ If it is nil the selected frame is used instead."
>    :type 'boolean)
>
>  (defcustom filechooser-filters `(("Directories" filechooser-file-directory-p . t)
> -                              ("Elisp files" ,(rx ".el" eos))
> -                              ("Not dot files" ,(rx bos (not ?.))))
> +				 ("Elisp files" ,(rx ".el" eos))
> +				 ("Not dot files" ,(rx bos (not ?.))))
>    "An alist of (NAME FILTER . BOOL).
>  NAME should describe the filter which can either be a regexp
>  or else a predicate function which takes a filename as argument.
> @@ -89,8 +92,8 @@ If BOOL is non-nil filter is active by default otherwise it is inactive."
>    :type '(alist :key-type (string :tag "Name")
>                  :value-type
>                  (cons :tag "Value:"
> -                  (choice :tag "Filter" regexp function)
> -                  (boolean :tag "Default"))))
> +                      (choice :tag "Filter" regexp function)
> +                      (boolean :tag "Default"))))
>
>  (defcustom filechooser-choose-file #'filechooser-read-file-name
>    "Function used to choose a single file.
> @@ -114,7 +117,7 @@ It should have the same calling convention as
>    "The key that is used to exit minibuffer to do completion.
>  I.e. the key that binds the equivalent of `exit-minibuffer' for the completion
>  UI of choice: usually RET."
> -  :type 'key)
> +  :type 'key)				;this is not available for Emacs 27

Will (if (> emacs-major-version 27) 'key 'string) be fine here?

>  ;;;; Others
>  (defvar filechooser-current-operation nil
> @@ -122,12 +125,12 @@ UI of choice: usually RET."
>
>  ;;;; Internal Variables
>  (defvar filechooser--filters nil)
> -(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t)))
> +(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t))) ;do you really want to evaluate a side effect here?

This is a hack to get a default-directory I can use for a fake dired
buffer. I think it is enough to create such a directory once when the
package is loaded. I will try changing it so that the existence of this
directory is checked at the point of use and if not it is created.

>  (defvar filechooser--multiple-selection nil)
>
>  ;;; Filters
>  (defun filechooser--filters-group-fn (cand transform)
> -  "Group function for selecting filters. CAND TRANSFORM."
> +  "Group function for selecting filters.  CAND TRANSFORM." ;please document CAND and TRANSFORM

Will do.

>    (if transform
>        cand
>      (if (cdr (alist-get cand filechooser--filters nil nil #'equal))

Thanks,
Rahguzar

Message #26 received at 68915 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Rahguzar <rahguzar <at> zohomail.eu>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 68915 <at> debbugs.gnu.org
Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Sun, 04 Feb 2024 16:33:26 -0500

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

Indeed.  Maybe it would make sense to have a feature request to be able
to write just:

    (rx bos (= (abs level) (: (1+ (not ".")) (?? "."))))

but currently I think you have to write that as

    (rx bos (regexp (rx-to-string `(= ,(abs level) (: (1+ (not ".")) (?? "."))))))

which is unsatisfactory.  You could make it marginally more efficient with:

    (rx bos (regexp (rx-to-string `(= ,(abs level) (regexp ,(rx (: (1+ (not ".")) (?? "."))))))))

but it's yet less satisfactory from a readability point of view.

>> -       (string-match (rx-to-string
>> -                      `(: (= ,(abs level) (: (1+ (not ".")) (?? "."))) eos))
>> +       (string-match (rx (= (literal (abs level)) (: (1+ (not ".")) (?? "."))) eos)

Same here.

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

Indeed, I suspect it should be

    (let* ((restriction-rx (rx (: (regexp (if (not arg)
                                              (rx (or "+" "-"))
                                            (if (> arg 0) (rx "+") (rx "-")))
                                  (0+ (not space))
                                  (or (1+ space) eos)))))

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

IIUC there's another such protocol for Company, but there isn't one
that's "generic" :-(


        Stefan

Message #29 received at 68915 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Rahguzar <rahguzar <at> zohomail.eu>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 68915 <at> debbugs.gnu.org
Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Sun, 04 Feb 2024 16:45:00 -0500

>> The better solution is to send a patch for `bytecomp.el`, of course, but
>> in this specific case you could workaround the problem with something
>> like the (guaranteed 100% untested) patch below.  It's not very pretty,
>> so I recommend you look into that `bytecomp.el` patch 🙂.
> That will probably require some CS education :)

My job is to provide that, so you're at the right place.

> I am not a coder by trade rather an aspiring academic in
> another field.

No worries: nowadays a CS education can be put to good use in pretty
much all fields.


        Stefan

Message #32 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 07:06:00 +0000

Rahguzar <rahguzar <at> zohomail.eu> writes:

> Hi Philip,
>   Thanks again for these.
>
> I will make most of the stylistic changes but I would like to not mix
> tabs and spaces for the code I want to maintain. Other than that some
> responses to your questions are below:

That was my mistake, I just ran indent-region on the entire buffer.  If
you want to avoid issues with this in the future, I'd recommend
adding a .dir-locals.el file that disables indent-tabs-mode.

I usually have this in my packages,

--8<---------------cut here---------------start------------->8---
;;; Directory Local Variables
;;; For more information see (info "(emacs) Directory Variables")

((emacs-lisp-mode
  (indent-tabs-mode . nil)
  (show-trailing-whitespace . t)
  (sentence-end-double-space . t)
  (byte-compile-error-on-warn . t)))
--8<---------------cut here---------------end--------------->8---

though I know that some people think it is opinionated.

> Philip Kaludercic <philipk <at> posteo.net> writes:
>
>> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>
>>>>> 2) filechooser.el: https://codeberg.org/rahguzar/filechooser
>>>>> It implements the backend D-bus methods for xdg filechooser and can be
>>>>> used to provide an Emacs based file selection interface for applications
>>>>> such as web browsers which support using xdg desktop portals for this
>>>>> purpose.
>>
>> I don't really understand what this package is doing, so my comments are
>> more superficial, hope they are nevertheless at least a bit useful:
>>
>> diff --git a/filechooser.el b/filechooser.el
>> index cc96386..e08f4b1 100644
>> --- a/filechooser.el
>> +++ b/filechooser.el
>> @@ -1,26 +1,27 @@
>>  ;;; filechooser.el --- An xdg-desktop-portal filechooser -*- lexical-binding: t; -*-
>> -;;
>> -;; Copyright (C) 2023 azeem
>> -;;
>> +
>> +;; Copyright (C) 2023 azeem <-- is this you?
>> +
>
> Yes, sorry will change that.
>
>>  ;; Author: rahguzar <rahguzar <at> zohomail.eu>
>>  ;; Maintainer: rahguzar <rahguzar <at> zohomail.eu>
>>  ;; Created: May 20, 2023
>> -;; Modified: May 20, 2023
>> +;; Modified: May 20, 2023 <-- either use time-stamp or remove this please
>>  ;; Version: 0.0.1
>>  ;; Keywords: convenience files tools unix
>>  ;; Homepage: https://codeberg.org/rahguzar/filechooser
>>  ;; Package-Requires: ((emacs "28.1") (compat "29.1"))
>> -;;
>> +
>>  ;; This file is not part of GNU Emacs.
>> -;;
>> +
>>  ;;; Commentary:
>> -;; An implementation of xdg-desktop-portal filechooser in Emacs. This allows
>> +
>> +;; An implementation of xdg-desktop-portal filechooser in Emacs.  This allows
>>  ;; for choosing files in applications like firefox (with GTK_USE_PORTAL set)
>> -;; from an Emacs frame. The default is to use `read-file-name' for choosing
>> +;; from an Emacs frame.  The default is to use `read-file-name' for choosing
>>  ;; a single file and a pair of Dired buffers for choosing multiple files.
>> -;;
>> -;;
>> +
>>  ;;; Code:
>> +
>>  (require 'compat)
>>  (require 'dbus)
>>  (require 'xdg)
>> @@ -49,25 +50,27 @@
>>
>>  (defvar filechooser-multiple-selection-map
>>    (let ((map (make-sparse-keymap)))
>> +    (set-keymap-parent map filechooser-mininuffer-map)
>>      (define-key map (kbd "M-TAB") #'filechooser-multiple-continue)
>>      (define-key map (kbd "M-RET") #'filechooser-multiple-finalize-current-selection)
>>      (define-key map (kbd "M-a") #'filechooser-multiple-select-all)
>> -    (make-composed-keymap map filechooser-mininuffer-map)))
>> +    map))
>>
>>  ;;;; Custom variables
>> -(defgroup filechooser nil
>> +(defgroup filechooser nil		;this is a duplicate!
>
> Is the other definition in some package? I didn't see any filechooser
> group is options for `customize-group` without this package loaded.

I believe there was a duplicate definition a few lines above.

>>    "An xdg-desktop-portal filechooser."
>>    :link '(url-link :tag "Homepage" "https://codeberg.org/rahguzar/filechooser")
>>    :group 'files)
>>
>>  (defcustom filechooser-save-existing-files 'uniquify
>>    "Determines behavior when attempting to save an existing file FILENAME.
>> -If it is symbol `yes-or-no-p', `yes-or-no-p' is used to confirm if the file
>> -should be overwritten. If it is the symbol `y-or-n-p', `y-or-n-p' is used to
>> -prompt. In both cases if a the answer is negative, the file selection is started
>> -again. If it is the symbol `uniquify', the FILENAME is made unique by appedning
>> --N to it where N is a positive number. If it is a function, it is called with
>> -FILENAME and the return value is used as the filename."
>> +If it is symbol `yes-or-no-p', `yes-or-no-p' is used to confirm if the
>> +file should be overwritten.  If it is the symbol `y-or-n-p', `y-or-n-p'
>> +is used to prompt.  In both cases if a the answer is negative, the file
>> +selection is started again.  If it is the symbol `uniquify', the
>> +FILENAME is made unique by appedning -N to it where N is a positive
>> +number.  If it is a function, it is called with FILENAME and the return
>> +value is used as the filename."
>>    :type '(choice
>>            (const :tag "Uniquify" uniquify)
>>            (const :tag "Prompt Yes/No" yes-or-no-p)
>> @@ -80,8 +83,8 @@ If it is nil the selected frame is used instead."
>>    :type 'boolean)
>>
>>  (defcustom filechooser-filters `(("Directories" filechooser-file-directory-p . t)
>> -                              ("Elisp files" ,(rx ".el" eos))
>> -                              ("Not dot files" ,(rx bos (not ?.))))
>> +				 ("Elisp files" ,(rx ".el" eos))
>> +				 ("Not dot files" ,(rx bos (not ?.))))
>>    "An alist of (NAME FILTER . BOOL).
>>  NAME should describe the filter which can either be a regexp
>>  or else a predicate function which takes a filename as argument.
>> @@ -89,8 +92,8 @@ If BOOL is non-nil filter is active by default otherwise it is inactive."
>>    :type '(alist :key-type (string :tag "Name")
>>                  :value-type
>>                  (cons :tag "Value:"
>> -                  (choice :tag "Filter" regexp function)
>> -                  (boolean :tag "Default"))))
>> +                      (choice :tag "Filter" regexp function)
>> +                      (boolean :tag "Default"))))
>>
>>  (defcustom filechooser-choose-file #'filechooser-read-file-name
>>    "Function used to choose a single file.
>> @@ -114,7 +117,7 @@ It should have the same calling convention as
>>    "The key that is used to exit minibuffer to do completion.
>>  I.e. the key that binds the equivalent of `exit-minibuffer' for the completion
>>  UI of choice: usually RET."
>> -  :type 'key)
>> +  :type 'key)				;this is not available for Emacs 27
>
> Will (if (> emacs-major-version 27) 'key 'string) be fine here?

I think 'key-sequence would be the more adequate fallback.

>>  ;;;; Others
>>  (defvar filechooser-current-operation nil
>> @@ -122,12 +125,12 @@ UI of choice: usually RET."
>>
>>  ;;;; Internal Variables
>>  (defvar filechooser--filters nil)
>> -(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t)))
>> +(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t))) ;do you really want to evaluate a side effect here?
>
> This is a hack to get a default-directory I can use for a fake dired
> buffer. I think it is enough to create such a directory once when the
> package is loaded. I will try changing it so that the existence of this
> directory is checked at the point of use and if not it is created.

Right, that sounds more robust.  I don't know all the edge cases in
which a temporary directory can be removed, especially over different
operating systems, so I'd recommend not relying on it, especially over
longer periods.

>>  (defvar filechooser--multiple-selection nil)
>>
>>  ;;; Filters
>>  (defun filechooser--filters-group-fn (cand transform)
>> -  "Group function for selecting filters. CAND TRANSFORM."
>> +  "Group function for selecting filters.  CAND TRANSFORM." ;please document CAND and TRANSFORM
>
> Will do.

Thanks.

>>    (if transform
>>        cand
>>      (if (cdr (alist-get cand filechooser--filters nil nil #'equal))
>
> Thanks,
> Rahguzar

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

Message #38 received at 68915 <at> debbugs.gnu.org (full text, mbox):

From: Rahguzar <rahguzar <at> zohomail.eu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 68915 <at> debbugs.gnu.org
Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Mon, 05 Feb 2024 22:33:33 +0100

Hi Stefan

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>> The better solution is to send a patch for `bytecomp.el`, of course, but
>>> in this specific case you could workaround the problem with something
>>> like the (guaranteed 100% untested) patch below.  It's not very pretty,
>>> so I recommend you look into that `bytecomp.el` patch 🙂.
>> That will probably require some CS education :)
>
> My job is to provide that, so you're at the right place.

Great! An item has been added to my post retirement aspirations :)

>> I am not a coder by trade rather an aspiring academic in
>> another field.
>
> No worries: nowadays a CS education can be put to good use in pretty
> much all fields.
>
>
>         Stefan

I believe I have taken care of most of the feedback from you and Philip
including gitignore and elpaignore files.

I think copyright stuff is also taken care of and I have removed "not"
from

;; This file is part of GNU Emacs.

line.

I will try to improve the let over lambda parts too but probably after
sometime.

Thanks for suggestions,
Rahguzar

Message #41 received at 68915 <at> debbugs.gnu.org (full text, mbox):

From: Rahguzar <rahguzar <at> zohomail.eu>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Philip Kaludercic <philipk <at> posteo.net>,
 Stefan Kangas <stefankangas <at> gmail.com>, 68915 <at> debbugs.gnu.org
Subject: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Mon, 05 Feb 2024 22:40:33 +0100

Hi Stefan and Philip,

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>>> -       (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.
>
> Indeed.  Maybe it would make sense to have a feature request to be able
> to write just:
>
>     (rx bos (= (abs level) (: (1+ (not ".")) (?? "."))))
>
> but currently I think you have to write that as
>
>     (rx bos (regexp (rx-to-string `(= ,(abs level) (: (1+ (not ".")) (?? "."))))))
>
> which is unsatisfactory.  You could make it marginally more efficient with:
>
>     (rx bos (regexp (rx-to-string `(= ,(abs level) (regexp ,(rx (: (1+ (not ".")) (?? "."))))))))
>
> but it's yet less satisfactory from a readability point of view.
>
>>> -       (string-match (rx-to-string
>>> -                      `(: (= ,(abs level) (: (1+ (not ".")) (?? "."))) eos))
>>> +       (string-match (rx (= (literal (abs level)) (: (1+ (not ".")) (?? "."))) eos)
>
> Same here.
>
>>> -  (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.
>
> Indeed, I suspect it should be
>
>     (let* ((restriction-rx (rx (: (regexp (if (not arg)
>                                               (rx (or "+" "-"))
>                                             (if (> arg 0) (rx "+") (rx "-")))
>                                   (0+ (not space))
>                                   (or (1+ space) eos)))))
>

For now I have left the regexps are they are except for the useless (or
(1+ space) eos) form that Philip caught. I think they are cleaner
without mixing rx and rx-to-string. They are used in interactive
commands for editing minibuffer strings so I think performance is not a
concern.

>> 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.
>
> IIUC there's another such protocol for Company, but there isn't one
> that's "generic" :-(
>
>
>         Stefan

Thanks again,
Rahguzar

Message #44 received at 68915 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Mon, 05 Feb 2024 22:45:45 +0100

Hi Philip,

Philip Kaludercic <philipk <at> posteo.net> writes:

>> Is the other definition in some package? I didn't see any filechooser
>> group is options for `customize-group` without this package loaded.
>
> I believe there was a duplicate definition a few lines above.
>

Thanks a lot for catching this. Should be fixed now.

>>> @@ -114,7 +117,7 @@ It should have the same calling convention as
>>>    "The key that is used to exit minibuffer to do completion.
>>>  I.e. the key that binds the equivalent of `exit-minibuffer' for the completion
>>>  UI of choice: usually RET."
>>> -  :type 'key)
>>> +  :type 'key)				;this is not available for Emacs 27
>>
>> Will (if (> emacs-major-version 27) 'key 'string) be fine here?
>
> I think 'key-sequence would be the more adequate fallback.

Thanks, changed now.

>>>  ;;;; Others
>>>  (defvar filechooser-current-operation nil
>>> @@ -122,12 +125,12 @@ UI of choice: usually RET."
>>>
>>>  ;;;; Internal Variables
>>>  (defvar filechooser--filters nil)
>>> -(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t)))
>>> +(defvar filechooser--selection (list (make-temp-file "filechooser-selection-" t))) ;do you really want to evaluate a side effect here?
>>
>> This is a hack to get a default-directory I can use for a fake dired
>> buffer. I think it is enough to create such a directory once when the
>> package is loaded. I will try changing it so that the existence of this
>> directory is checked at the point of use and if not it is created.
>
> Right, that sounds more robust.  I don't know all the edge cases in
> which a temporary directory can be removed, especially over different
> operating systems, so I'd recommend not relying on it, especially over
> longer periods.
>

I have changed this too.

>>>  (defvar filechooser--multiple-selection nil)
>>>
>>>  ;;; Filters
>>>  (defun filechooser--filters-group-fn (cand transform)
>>> -  "Group function for selecting filters. CAND TRANSFORM."
>>> +  "Group function for selecting filters.  CAND TRANSFORM." ;please document CAND and TRANSFORM
>>
>> Will do.
>
> Thanks.
>

I have referred to info node. Is is possible to refer to a specific
paragraph in the info manual? Ideally I would like to point to the text
about group function there.

>>>    (if transform
>>>        cand
>>>      (if (cdr (alist-get cand filechooser--filters nil nil #'equal))
>>
>> Thanks,
>> Rahguzar

Thanks a lot,
Rahguzar

Message #47 received at 68915 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Mon, 05 Feb 2024 22:51:21 +0100

Philip Kaludercic <philipk <at> posteo.net> writes:

> Rahguzar <rahguzar <at> zohomail.eu> writes:
>
> 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.
>

I have defined a new group now.

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

The documentation mentions the rationale for using the same buffer
throughout. Is that not adequate?

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

Thinking about this, that usage of vertico count wasn't doing anything.
So I have just used a constant height for window. Customizing the
preview window is (and was) possible through `display-buffer-alist`

Thanks a lot for comments and suggestions,
Rahguzar

Message #50 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: Tue, 06 Feb 2024 09:43:26 +0000

Rahguzar <rahguzar <at> zohomail.eu> writes:

>>>>  (defvar filechooser--multiple-selection nil)
>>>>
>>>>  ;;; Filters
>>>>  (defun filechooser--filters-group-fn (cand transform)
>>>> -  "Group function for selecting filters. CAND TRANSFORM."
>>>> +  "Group function for selecting filters.  CAND TRANSFORM." ;please document CAND and TRANSFORM
>>>
>>> Will do.
>>
>> Thanks.
>>
>
> I have referred to info node. Is is possible to refer to a specific
> paragraph in the info manual? Ideally I would like to point to the text
> about group function there.

Not to my knowledge, the granularity of links is limited to notes.

Message #53 received at 68915 <at> debbugs.gnu.org (full text, mbox):

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: Re: bug#68915: Request to include a couple of packages in GNU ELPA
Date: Sun, 25 Feb 2024 17:21:40 +0100

Hi Philip,
Philip Kaludercic <philipk <at> posteo.net> writes:

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

On the hoogle-buffer branch of the repositories,

https://codeberg.org/rahguzar/consult-hoogle/src/branch/hoogle-buffer

there is a command hoogle-buffer which displays results in a fixed
buffer. The package depends on consult for now but I will change it to
an optional dependency when I merge this on the main branch.

Rahguzar

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.