Re: [PATCH] DOCT integration: template specific hooks [9.5.5 (9.5.5-g003cc7 @ /home/n/.emacs.d/elpaca/builds/org/)]

[Ihor Radchenko <yantar92@gmail.com>]

No Wayman <iarchivedmywholelife@gmail.com> writes:

> The attached patch is the first step toward integrating DOCT[1] 
> syntax into Org mode.

Thanks! This is most welcome.

> It adds property options to org-capture-templates which make it 
> easier to run template-specific hooks.
> ...
> Contrast the above with the following syntax enabled by the 
> attached patch: 
> 
> #+begin_src emacs-lisp :lexical t
> (let ((org-capture-templates
>        '(("t" "test" plain (file "/tmp/test.org")
> 	  "test %?"
> 	  :hook ((lambda () (insert "mode-hook\n")))
> 	  :before-finalize ((lambda () (insert 
> 	  "before-finalize\n")))
> 	  ;; Only a message because this happens outside the 
> 	  context
> 	  ;; of the capture buffer.
> 	  :after-finalize ((lambda () (message "after-finalize")))
> 	  :prepare-finalize ((lambda () (insert 
> 	  "prepare-finalize\n")))))))
>   (org-capture nil "t"))
> #+end_src
> 
> These template-specific hook functions run prior to their global 
> counterparts.

LGTM.
 
> Ihor, an implementation note: I have not used `run-hooks' with 
> these because they have no associated symbol.
> The functions are lists stored directly on `org-capture-plist'.

Then, please add an appropriate comment in the code.

> See attached patch.
>
> From 780194a5af6a8a6c7fbb602e834dcbec78016070 Mon Sep 17 00:00:00 2001
> From: Nicholas Vollmer <iarchivedmywholelife@gmail.com>
> Date: Tue, 27 Sep 2022 05:44:33 -0400
> Subject: [PATCH] org-capture: Add template hook properties
>
> * lisp/org-capture.el (org-capture-templates): Document template hook properties.
> (org-capture-finalize): execute :prepare/:before/:after-finalize functions.
Nitpick: Execute

> (org-capture-place-template): execute :hook functions.

> * doc/org-manual.org Document template hook properties.

You also need to add an appropriate ORG-NEWS entry.

> +  - ~:hook~ ::
> +
> +    A function or list of functions run before `org-capture-mode-hook'
> +    when the template is selected.

You did not mention the number of arguments passed to the functions.

> + - ~:prepare-finalize~ ::
> +
> +    A function or list of functions run before `org-capture-prepare-finalize-hook'
> +    when the template is selected.
> +
> + - ~:before-finalize~ ::
> +
> +    A function or list of functions run before `org-capture-before-finalize-hook'
> +    when the template is selected.
> +
> + - ~:after-finalize~ ::
> +
> +    A function or list of functions run before `org-capture-after-finalize-hook'
> +    when the template is selected.

In the above, please use the quoting consistently. This is an Org file,
so do not use `symbol'. Instead, use ~symbol', as documented in
doc/Documentation_Standards.org

>   :no-save            Do not save the target file after finishing the capture.
>  
> + :hook               A function or list of functions run before
> +                     `org-capture-mode-hook' when the template is selected.
> +
> + :prepare-finalize   A function or list of functions run before
> +                     `org-capture-prepare-finalize-hook'
> +                     when the template is selected.
> +
> + :before-finalize    A function or list of functions run before
> +                     `org-capture-before-finalize-hook'
> +                     when the template is selected.
> +
> + :after-finalize     A function or list of functions run before
> +                     `org-capture-after-finalize-hook'
> +                     when the template is selected.

Again, the number of arguments passed to the hooks should be documented.

>  
> +(defun org-capture--run-template-functions (keyword &optional local)
> +  "Run template's KEYWORD functions.
> +If LOCAL is non-nil use the buffer-local value of `org-capture-plist'."

It is unclear what "KEYWORD functions" refers to.

> +  (let ((value (org-capture-get keyword local)))
> +    (if (functionp value)
> +        (funcall value)
> +      (mapc #'funcall value))))

The reasoning why not `run-hooks' might be commented here.

>  
> +    ;; Do not use the local arg to `org-capture-get' here.
> +    ;; The buffer-local value has been stored on `org-capture-plist'.

Did you mean "... local arg to `org-capture--run-template-function'"?

> +    (org-capture--run-template-functions :after-finalize)
>      (run-hooks 'org-capture-after-finalize-hook)
>      ;; Special cases
>      (cond
> @@ -1147,6 +1175,7 @@ may have been stored before."
>      (`item (org-capture-place-item))
>      (`checkitem (org-capture-place-item)))
>    (setq-local org-capture-current-plist org-capture-plist)
> +  (org-capture--run-template-functions :hook t)
>    (org-capture-mode 1))

Here and above, it would be a bit more clear to use something like
'local instead of t argument.

Also, please add some tests into testing/lisp/test-org-capture.el.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
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>