> Would you consider it? See > https://orgmode.org/worg/org-contribute.html#copyright
I have already done the copyright assignment.
> Please add a proper commit message, if you can.
> We also need an ORG-NEWS entry announcing the new customization.
> "carefully set craft regexp..." reads as if Org will arrange this, not
> This docstring is too long. Please make sure that the first line of
> the
> Nickpick: pcase of cl-case will be shorter
This new patch tried to address them.
> > + (let* ((cands (hash-table-keys completion-hash-table))
> > + ;; Handle regexp separator?
>
> Is it a TODO?
>
> > + (dyn-sep separator)
> > + (consec-sep-regexp (format "\\(%s\\)+" separator)))
>
> regexp-opt will be safer if separator happens to be something like
> ".".
Previously I didn't think thoroughly on how to handle regexp-based
separators. Now that you suggested regexp-opt for the "." case so I
opted to handle literal separators only, currently hard-coded as ";"
anyway.
> dlet is unnecessary here. A normal let will do because `crm-separator'
> is already a dynamically scoped variable.
I was thinking what if crm.el isn't already loaded? org.el has
defvar-ed crm-separator but a test like this would still give an
unintended separator:
Write at /tmp/test-dlet-crm.el:
```elisp
;; -*- lexical-binding: t; -*-
(require 'org)
(require 'oc)
(defun my-crm-separator-literal ()
(replace-regexp-in-string "\\`\\[.*?]\\*\\|\\[.*?]\\*\\'" "" crm-separator))
(defun my-crm-indicator--filter-args-a (args)
(cons (format "[CRM%s] %s" (my-crm-separator-literal) (car args)) (cdr args)))
(advice-add
#'completing-read-multiple
:filter-args #'my-crm-indicator--filter-args-a)
(defun foo ()
(interactive)
(let ((crm-separator "[ ]*;[ ]*"))
(message "%S" (completing-read-multiple "Hi: " '("a" "b" "c") nil t))))
```
Then run
emacs /tmp/test.org -l /tmp/test-dlet-crm.el
M-x foo
The first invocation of foo would prompt "[CRM,] ..." instead of the
intended "[CRM;] ...".
The previous patch worked for me but I'm still not skeptical about the
dynamic option's code quality. Is inserting candidates in a temporary
buffer separated by newline then search on them really the
optimal/flexible way?
Daanturo
On Mar 18 2025, at 12:00 am, Ihor Radchenko <yanta...@posteo.net> wrote:
>
> First of all, thanks for providing a patch!
> However, please note that I cannot accept patches exceeding 15-20LOC
> without copyright assignment.
> Would you consider it? See
> https://orgmode.org/worg/org-contribute.html#copyright
>
> Some comments on the patch below.
> > Subject: [PATCH] org-cite-basic-complete-key-crm-separator
> Please add a proper commit message, if you can.
> See https://orgmode.org/worg/org-contribute.html#commit-messages
>
> We also need an ORG-NEWS entry announcing the new customization.
> > +(defcustom org-cite-basic-complete-key-crm-separator nil
> > + "When non-nil, use `completing-read-multiple' with this as the separator.
> > +When nil: use traditional consecutive prompts. When set to a
> > +string (regexp), carefully set craft a regexp that doesn't appear in the
>
> "carefully set craft regexp..." reads as if Org will arrange this, not
> user. I think a simpler wording would be
>
> When nil, use multiple `completing-read' prompts. When set to a
> string, it should be a regexp to be used as `crm-separator' (which
> see). When set to symbol `dynamic', use ";;..." as a separator with
> the number of ";" sufficient so that none of the completion candidates
> contain the separator.
>
> > +(defun org-cite-basic--complete-key-dynamic-crm-separator
> > + (completion-hash-table separator)
> > + "Return a repeated version of SEPARATOR as needed that doesn't appear in
> > COMPLETION-HASH-TABLE."
>
> This docstring is too long. Please make sure that the first line of the
> docstring does not exceed 72 symbols.
>
> > + (let* ((cands (hash-table-keys completion-hash-table))
> > + ;; Handle regexp separator?
>
> Is it a TODO?
> > + (dyn-sep separator)
> > + (consec-sep-regexp (format "\\(%s\\)+" separator)))
>
> regexp-opt will be safer if separator happens to be something like ".".
> > + (dlet ((crm-separator
> dlet is unnecessary here. A normal let will do because `crm-separator'
> is already a dynamically scoped variable.
>
> > + (cond
> > + ((stringp org-cite-basic-complete-key-crm-separator)
> > + org-cite-basic-complete-key-crm-separator)
> > + ((equal
> > + 'dynamic org-cite-basic-complete-key-crm-separator)
> > + (org-cite-basic--complete-key-dynamic-crm-separator
> > + table ";")))))
>
> Nickpick: pcase of cl-case will be shorter
> >> We can use something like ;; as a separator or even compute it
> >> dynamically depending on the actual completion candidates (; if no
> >> candidates contain ;, ;;, ;;;, etc)
> >
> > How does this look? I haven't known a function to reliably query all
> > completion candidates from a completion table of regardless of its type.
> >
> > The ability to dynamically compute the crm-separator based on the
> > candidates sounds like a useful feature. I think it's best for Emacs
> > core to have that (optional) functionality built-in.
>
> We can discuss such addition on emacs-devel, if you are willing to. It
> will likely require more work though to make things more general.
>
> --
> Ihor Radchenko // yantar92,
> Org mode maintainer,
> Learn more about Org mode at <https://orgmode.org/>.
> Support Org development at <https://liberapay.com/org-mode>,
> or support my work at <https://liberapay.com/yantar92>
>
0001-oc-basic.el-new-option-org-cite-basic-complete-key-crm-separator.patch
Description: Binary data
