Re: [O] [patch] structure snippet completions

[Nicolas Goaziou]

Hello,

Rasmus <address@hidden> writes:

> The attached patch adds expansions of "<s"-like snippets to Org v9.2.  I
> would like to include this in the next version of Org in anticipation of
> the changes to the template system.

Thank you. Some, mostly cosmetics, comments follow.
>
> In at least emacs-git it "just works" after requiring org-tempo.
>
> Since it currently uses with-eval-after-load it would require Emacs 24.4,
> but this could be changed to eval-after-load if necessary.

We officially support Emacs 24.3, even though some tests fail. IOW, you
need to use `eval-after-load'.

> * lisp/org-tempo.el: New file.
> * doc/org.texi (Structure templates):
> * etc/ORG-NEWS: Document new library.

No need to document ORG-NEWS changes.

> address@hidden org-tempo

Is it worth mentioning the _variable_ `org-tempo'?

Also,

@cindex Tempo
@cindex Template expansion
@cindex ... whatever ...

> address@hidden org-tempo-keywords-alist

Is 

@vindex org-structure-template-alist

missing?

> address@hidden can be used to expand snippets to structures defined
> in

Org Tempo expands snippets...

> address@hidden and @code{org-tempo-keywords-alist}.  For
> +example, @code{org-tempo} makes @kbd{<} @kbd{s} @address@hidden
> expand to a

For example, Org Tempo makes @kbd(< s @key{TAB}) expand to a code block.

> address@hidden code block.  Enable it by customizing @code{org-modules} or add
> address@hidden(require 'org-tempo)} to your Emacs init address@hidden more
> +information, please refer to the commentary section in
> @code{org-tempo.el}}.

... @file{org-tempo.el}.}.

> +;;; org-tempo.el --- template expansion for org structures -*- 
> lexical-binding: t; -*-

Template expansion for Org structures

> +;; org-tempo reimplements completions of structure template before

Org Tempo implements...

> +(defvar org-tempo-tags nil
> +  "Tempo tags for org-mode")

"Tempo tags for Org mode."

> +(defcustom org-tempo-keywords-alist
> +  '((?L . "latex")
> +    (?H . "html")
> +    (?A . "ascii")
> +    (?i . "index"))
> +  "Keyword templates like `org-structure-template-alist'."

IMO, the docstring doesn't say much for someone discovering the feature.

> +  :group 'org-tempo

You didn't define the `org-tempo' group, did you?

> +  :type '(repeat
> +       (cons (character :tag "Key")
> +             (string :tag "Template")))
> +  :package-version '(Org . "9.2"))
> +
> +(defun org-tempo-setup ()
> +  (org-tempo-add-templates)
> +  (tempo-use-tag-list 'org-tempo-tags)
> +  (setq-local tempo-match-finder "^ *\\(<[[:word:]]\\)\\="))
> +
> +(defun org-tempo-add-templates ()
> +  "Update all org-tempo templates.

"Update all Org Tempo templates."

> +Goes through `org-structure-template-alist' and
> +`org-tempo-keywords-alist'."
> +  (let ((keys (mapcar (apply-partially 'format "<%c")

#'format

> +                   (mapcar 'car (append org-structure-template-alist
> +                                        org-tempo-keywords-alist)))))

#'car

but I think the following is simpler:

(mapcar (lambda (pair) (format "<%s" (car pair)))
        (append org-structure-template-alist
                org-tempo-keywords-alist))

> +    (if (> (length keys)
> +           (length (delete-dups keys)))

`when'

> +     (user-error
> +      "Duplicated keys in `org-structure-template-alist' and
> `org-tempo-keywords-alist'"))

Is is an issue?

> +    (mapcar (lambda (key)
> +           (if (assoc-string key org-tempo-tags)
> +               (setq org-tempo-tags
> +                     (delete (assoc-string key org-tempo-tags)
> +                             org-tempo-tags))))
> +         keys)
> +    (mapcar 'org-tempo-add-block org-structure-template-alist)

#'org-tempo-add-block

> +    (mapcar 'org-tempo-add-keyword org-tempo-keywords-alist)))

#'org-tempo-add-keyword

> +
> +(defun org-tempo-add-block (entry)
> +  "Add block entry from `org-structure-template-alist'."
> +  (let* ((key (format "<%c" (car entry)))
> +      (name (cdr entry)))
> +    (tempo-define-template (format "org-%s" (replace-regexp-in-string " " 
> "-" name))
> +                        `(,(format "#+begin_%s " name) p '> n n
> +                          ,(format "#+end_%s" (car (org-split-string name " 
> ")))
> +                          >)

`org-split-string' -> `split-string'

> +                        key
> +                        (format "Insert a %s block" name)
> +                        'org-tempo-tags)))
> +
> +(defun org-tempo-add-keyword (entry)
> +  "Add keyword entry from `org-tempo-keywords-alist'."
> +  (let* ((key (format "<%c" (car entry)))
> +      (name (cdr entry)))
> +    (tempo-define-template (format "org-%s" (replace-regexp-in-string " " 
> "-" name))
> +                        `(,(format "#+%s: " name) p '>)
> +                        key
> +                        (format "Insert a %s keyword" name)
> +                        'org-tempo-tags)))
> +
> +;;; Additional keywords
> +
> +(tempo-define-template "org-include"
> +                    '("#+include: "
> +                      (ignore-errors

Why `ignore-errors'?

> +           ;; Simple test if `org-tempo-setup' has been run.
> +           ;; May not be the case if `org-tempo' was loaded
> +           ;; after Org.
> +           (unless (cl-member "<I" tempo-collection :key 'car :test 'equal)
> +             (org-tempo-setup))

(unless (assoc "<I" tempos-collection) (org-tempo-setup))

But wouldn't calling 

  (org-tempo-setup)

at top level in "org-tempo.el" solve the issue?

Regards,

-- 
Nicolas Goaziou