org-ql-refile makes assumptions about org-refile-targets that conflict with the latter's docstring

[apc]

OS/platform

macOS

Emacs version and provenance

GNU Emacs 29.4 (build 2, aarch64-apple-darwin21.6.0, NS appkit-2113.65 Version 12.7.4 (Build 21H1123)) of 2024-08-19

Using emacs-plus

==> d12frosted/emacs-plus/emacs-plus@29: stable 29.4, HEAD
GNU Emacs text editor
https://www.gnu.org/software/emacs/
Installed
/opt/homebrew/Cellar/emacs-plus@29/29.4 (4,540 files, 255MB) *
  Built from source on 2024-08-19 at 15:15:19 with: --with-xwidgets --with-no-frame-refocus --with-native-comp --with-imagemagick --with-mailutils --with-modern-sexy-v2-icon
From: https://github.com/d12frosted/homebrew-emacs-plus/blob/HEAD/Formula/[email protected]

Emacs command

/opt/homebrew/bin/emacs -Q

Org version and provenance

Org mode version 9.6.15 (release_9.6.15 @ /opt/homebrew/Cellar/emacs-plus@29/29.4/share/emacs/29.4/lisp/org/)

org-ql package version and provenance

Version: 0.9-pre, acquired using straight via use-package

Actions taken

I called org-ql-refile on an org-mode heading.

Observed results

Saw the following error:

Symbol's function definition is void: +org-dump

Expected results

In my config, +org-dump is a variable whose value is a particular directory not among my org-agenda-files. I did not expect org-ql-refile would expect it to be a function. The variable appears in my configuration of the variable org-refile-targets, whose value in my config is as follows:

((nil :maxlevel . 5)
 (org-agenda-files :maxlevel . 3)
 (+org-dump :maxlevel . 2))

Backtrace

Debugger entered--Lisp error: (void-function +org-dump)
  +org-dump()
  byte-code("\301p\10\302\211\211\3:\203O\0\3@\262\2\1\211A\262\3\242\262\3\303\3\204!\0pC\202C\0\39\203=\0\3 \211;\2032\0\211C\2028\0..." [org-refile-targets delete-dups nil reverse org-ql-completing-read :prompt "Refile to: "] 9)
  command-execute(org-ql-refile record)
  execute-extended-command(nil "org-ql-refile" nil)
  funcall-interactively(execute-extended-command nil "org-ql-refile" nil)
  command-execute(execute-extended-command)

Etc.

By stepping through a call to org-ql-refile my understanding is that org-ql-refile is assuming that the car of each cons cell in org-refile-targets is a function (as far as I can tell, that's what this line suggests). But the docstring for org-refile-targets gives the user the option to use either a variable or a function:

Targets for refiling entries with M-x org-refile.

This is a list of cons cells.  Each cell contains:
- a specification of the files to be considered, either a list of files,
  or a symbol whose function or variable value will be used to retrieve
  a file name or a list of file names.  If you use org-agenda-files for
  that, all agenda files will be scanned for targets.  Nil means consider
  headings in the current buffer.
...

For now I'm just defining a function which outputs the current value of +org-dump. Perhaps that's the correct approach, but the documentation suggests that there's nothing wrong with my having a cons cell whose car is a variable. Indeed, org-refile works just fine in my system when +org-dump is just a variable.

[alphapapa]

Hi,

Thanks for the well-written bug report. Sounds like an oversight, indeed. I'll fix it when I get time, or patches welcome in the meantime.

[apc]

I don't feel competent enough to suggest a patch, but I'm thinking something like this minimal fix might suffice?

(defun org-ql-refile (marker)
  "Refile current entry to MARKER (interactively, one selected with `org-ql').
Interactive completion uses files listed in `org-refile-targets',
which see (but only the files are used)."
  (interactive (let ((buffers-files (delete-dups
                                     ;; Always include the current buffer.
                                     (cons (current-buffer)
                                           (cl-loop for (files-spec . _candidate-spec) in org-refile-targets
                                                    append (cl-typecase files-spec
                                                             (null (list (current-buffer)))
                                                             (symbol
                                                              (cond ((fboundp files-spec)
                                                                     (pcase (funcall files-spec)
                                                                       ((and (pred stringp) file) (list file))
                                                                       ((and (pred listp) files) files)))
                                                                    ((boundp files-spec)
                                                                     (pcase files-spec
                                                                       ((and (pred stringp) file) (list file))
                                                                       ((and (pred listp) files) files)))))
                                                             (list files-spec)))))))
                 (list (org-ql-completing-read buffers-files :prompt "Refile to: "))))
  (let ((buffer (or (buffer-base-buffer (marker-buffer marker))
                    (marker-buffer marker))))
    (org-refile nil nil
                ;; The RFLOC argument:
                (list
                 ;; Name
                 (org-with-point-at marker
                   (nth 4 (org-heading-components)))
                 ;; File
                 (buffer-file-name buffer)
                 ;; nil
                 nil
                 ;; Position
                 marker))))

[alphapapa]

cl-typecase can have function clauses, which can be used to test the type rather than fboundp. See (info "(cl) Type Predicates").

[apc]

Would this be better?

(defun org-ql-refile (marker)
  "Refile current entry to MARKER (interactively, one selected with `org-ql').
Interactive completion uses files listed in `org-refile-targets',
which see (but only the files are used)."
  (interactive (let ((buffers-files (delete-dups
                                     ;; Always include the current buffer.
                                     (cons (current-buffer)
                                           (cl-loop for (files-spec . _candidate-spec) in org-refile-targets
                                                    append (cl-typecase files-spec
                                                             (null (list (current-buffer)))
                                                             (function (pcase (funcall files-spec)
                                                                         ((and (pred stringp) file) (list file))
                                                                         ((and (pred listp) files) files)))
                                                             (symbol (pcase (eval files-spec)
                                                                       ((and (pred stringp) file) (list file))
                                                                         ((and (pred listp) files) files)))
                                                             (list files-spec)))))))
                 (list (org-ql-completing-read buffers-files :prompt "Refile to: "))))
  (let ((buffer (or (buffer-base-buffer (marker-buffer marker))
                    (marker-buffer marker))))
    (org-refile nil nil
                ;; The RFLOC argument:
                (list
                 ;; Name
                 (org-with-point-at marker
                   (nth 4 (org-heading-components)))
                 ;; File
                 (buffer-file-name buffer)
                 ;; nil
                 nil
                 ;; Position
                 marker))))

[alphapapa]

Probably so. The best way to move forward would be to submit a pull request. :)