No Wayman <iarchivedmywholel...@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 <iarchivedmywholel...@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>
