Re: [patch] structure snippet completions

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Rasmus <rasmus@gmx.us> 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.

> +@vindex org-tempo

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

Also,

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

> +@vindex org-tempo-keywords-alist

Is 

@vindex org-structure-template-alist

missing?

> +@code{org-tempo} can be used to expand snippets to structures defined
> in

Org Tempo expands snippets...

> +@code{org-structure-template-alist} and @code{org-tempo-keywords-alist}.  For
> +example, @code{org-tempo} makes @kbd{<} @kbd{s} @kbd{@key{TAB}}
> expand to a

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

> +@samp{src} code block.  Enable it by customizing @code{org-modules} or add
> +@code{(require 'org-tempo)} to your Emacs init file@footnote{For 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