[PATCH] Capture: Expand keyword within %(SEXP) in template

[PATCH] Capture: Expand keyword within %(SEXP) in template

[Ryo TAKAISHI]

* lisp/org-capture.el: If %(SEXP) has %:keyword, expand it using org-store-link-plist.

I want to expand %:description keyword in sexp "%(func %:description)".
But if org-capture template is "%(function %:keyword)", function take a symbol %:keyword, it does'nt expand.
This patch expand %:keyword within %(SEXP), so funcsion is taken %:keyword's value.
For example, when capture template is "%(func %:description)" and a keyword :description is "foobar", func is taken string "foobar".

Modified from a patch proposal by Ryo TAKAISHI.

TINYCHANGE
---
 lisp/org-capture.el |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index be973b0..a8e49d6 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1620,7 +1620,15 @@ The template may still contain \"%?\" for cursor positioning."
       (goto-char (match-beginning 0))
       (let ((template-start (point)))
 	(forward-char 1)
-	(let ((result (org-eval (read (current-buffer)))))
+	(let* ((sexp (mapcar '(lambda (attr)
+				(let* ((attr-symbol (symbol-name attr))
+				       (key (if (string-match "%\\(:.*\\)" attr-symbol)
+						(intern (match-string 1 attr-symbol))
+					      nil)))
+				  (or (plist-get org-store-link-plist key)
+				      attr)))
+			     (read (current-buffer))))
+	       (result (org-eval sexp)))
 	  (delete-region template-start (point))
 	  (insert result))))))
 
-- 
1.7.9.5

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Nicolas Goaziou]

Hello,

Thanks for your patch. Here are a few comments about it.

Ryo TAKAISHI <ryo.takaishi.0@gmail.com> writes:

> * lisp/org-capture.el: If %(SEXP) has %:keyword, expand it using org-store-link-plist.
>
> I want to expand %:description keyword in sexp "%(func %:description)".
> But if org-capture template is "%(function %:keyword)", function take a symbol %:keyword, it does'nt expand.
> This patch expand %:keyword within %(SEXP), so funcsion is taken %:keyword's value.
> For example, when capture template is "%(func %:description)" and
> a keyword :description is "foobar", func is taken string "foobar".

I'm not sure to understand why this patch is necessary. Can't you use
(plist-get org-store-link-plist :description) from your sexp instead?

> +	(let* ((sexp (mapcar '(lambda (attr)

lambdas are self-quoting: do not explicitly quote them.

> +				       (key (if (string-match "%\\(:.*\\)" attr-symbol)
> +						(intern (match-string 1 attr-symbol))
> +					      nil)))

    (key (and (string-match "%\\(:.*\\)" attr-symbol)
              (intern (match-string 1 attr-symbol))))

is better.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Ryo TAKAISHI]

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Hello,
>
> Thanks for your patch. Here are a few comments about it.
>
> Ryo TAKAISHI <ryo.takaishi.0@gmail.com> writes:
>
>> * lisp/org-capture.el: If %(SEXP) has %:keyword, expand it using org-store-link-plist.
>>
>> I want to expand %:description keyword in sexp "%(func %:description)".
>> But if org-capture template is "%(function %:keyword)", function
>> take a symbol %:keyword, it does'nt expand.
>> This patch expand %:keyword within %(SEXP), so funcsion is taken %:keyword's value.
>> For example, when capture template is "%(func %:description)" and
>> a keyword :description is "foobar", func is taken string "foobar".
>
> I'm not sure to understand why this patch is necessary. Can't you use
> (plist-get org-store-link-plist :description) from your sexp instead?
>

I did'nt come up with to use it.
But "%(func %:description)" or "%(func (plist-get org-store-link-plist :description))", I think the former is readble template than the latter.

>> +	(let* ((sexp (mapcar '(lambda (attr)
>
> lambdas are self-quoting: do not explicitly quote them.
>
>> +				       (key (if (string-match "%\\(:.*\\)" attr-symbol)
>> +						(intern (match-string 1 attr-symbol))
>> +					      nil)))
>
>     (key (and (string-match "%\\(:.*\\)" attr-symbol)
>               (intern (match-string 1 attr-symbol))))
>
> is better.
>
>
> Regards,

Thank you for your comment, I'll refine my code.

Regards,
Ryo

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Nicolas Goaziou]

Ryo TAKAISHI <ryo.takaishi.0@gmail.com> writes:

> I did'nt come up with to use it.
> But "%(func %:description)" or "%(func (plist-get org-store-link-plist :description))", I think the former is readble template than the latter.

Probably, but it's also more error-prone.

For example, your code operates only at top-level, i.e. it won't handle
something like:

    %(fun arg1 (sub-fun %:description))

It will also error out if any atom isn't a symbol, i.e.:

    %(format "%d" 1)

It's all about pro and cons.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Ryo TAKAISHI]

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Ryo TAKAISHI <ryo.takaishi.0@gmail.com> writes:
>
>> I did'nt come up with to use it.
>> But "%(func %:description)" or "%(func (plist-get
>> org-store-link-plist :description))", I think the former is readble
>> template than the latter.
>
> Probably, but it's also more error-prone.
>
> For example, your code operates only at top-level, i.e. it won't handle
> something like:
>
>     %(fun arg1 (sub-fun %:description))
>
> It will also error out if any atom isn't a symbol, i.e.:
>
>     %(format "%d" 1)
>
> It's all about pro and cons.
>
>
> Regards,

I fix these problem. A new patch expand keyword recursively, and only symbol.

Regards,

---
 lisp/org-capture.el |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 1dfffc6..b8c9844 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1621,10 +1621,23 @@ The template may still contain \"%?\" for cursor positioning."
       (goto-char (match-beginning 0))
       (let ((template-start (point)))
 	(forward-char 1)
-	(let ((result (org-eval (read (current-buffer)))))
+	(let* ((sexp (mapcar 'org-capture-expand-keyword-in-embedded-elisp
+			     (read (current-buffer))))
+	       (result (org-eval sexp)))
 	  (delete-region template-start (point))
 	  (insert result))))))
 
+(defun org-capture-expand-keyword-in-embedded-elisp (attr)
+  (cond ((consp attr)
+	 (mapcar 'org-capture-expand-keyword-in-embedded-elisp attr))
+	((symbolp attr)
+	 (let* ((attr-symbol (symbol-name attr))
+		(key (and (string-match "%\\(:.*\\)" attr-symbol)
+			  (intern (match-string 1 attr-symbol)))))
+	   (or (plist-get org-store-link-plist key)
+	       attr)))
+	(t attr)))
+
 (defun org-capture-inside-embedded-elisp-p ()
   "Return non-nil if point is inside of embedded elisp %(sexp)."
   (let (beg end)
-- 
1.7.9.5

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Nicolas Goaziou]

Hello,

> I fix these problem. A new patch expand keyword recursively, and only
> symbol.

Great.

> +	(let* ((sexp (mapcar 'org-capture-expand-keyword-in-embedded-elisp
> +			     (read (current-buffer))))
> +	       (result (org-eval sexp)))
>  	  (delete-region template-start (point))
>  	  (insert result))))))
>  
> +(defun org-capture-expand-keyword-in-embedded-elisp (attr)
> +  (cond ((consp attr)
> +	 (mapcar 'org-capture-expand-keyword-in-embedded-elisp attr))
> +	((symbolp attr)
> +	 (let* ((attr-symbol (symbol-name attr))
> +		(key (and (string-match "%\\(:.*\\)" attr-symbol)
> +			  (intern (match-string 1 attr-symbol)))))
> +	   (or (plist-get org-store-link-plist key)
> +	       attr)))
> +	(t attr)))

The code looks good, although I would have moved the external `mapcar'
within the worker function in order to make its purpose clearer.

Also, would you mind to provide a docstring for the function? Something
along the lines of:

    "Recursively replace capture link keywords in ATTR sexp. 
  Such keywords are prefixed with "%:".  See `org-capture-template' for
  more information."

You may also want to rename it
`org-capture--expand-keyword-in-embedded-elisp' to insist on the fact it
is an internal function. That's not mandatory, though.

Finally, this feature must be documented in `org-capture-templates'
docstring and org.texi.

Thank you for working on it.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Ryo TAKAISHI]

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> The code looks good, although I would have moved the external `mapcar'
> within the worker function in order to make its purpose clearer.
>
> Also, would you mind to provide a docstring for the function? Something
> along the lines of:
>
>     "Recursively replace capture link keywords in ATTR sexp. 
>   Such keywords are prefixed with "%:".  See `org-capture-template' for
>   more information."
>
> You may also want to rename it
> `org-capture--expand-keyword-in-embedded-elisp' to insist on the fact it
> is an internal function. That's not mandatory, though.
>
> Finally, this feature must be documented in `org-capture-templates'
> docstring and org.texi.

Hello,

I fixed and added document in docstring & org.texi.
Is my additional document fully?

Regards,
Ryo

 doc/org.texi        |    2 ++
 lisp/org-capture.el |   20 +++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/doc/org.texi b/doc/org.texi
index b23f492..b661dac 100644
--- a/doc/org.texi
+++ b/doc/org.texi
@@ -6853,6 +6853,8 @@ dynamic insertion of content.  The templates are expanded in the order given her
 @smallexample
 %[@var{file}]     @r{Insert the contents of the file given by @var{file}.}
 %(@var{sexp})     @r{Evaluate Elisp @var{sexp} and replace with the result.}
+                  @r{If sexp's attr is link keyword (@code{%:keyword}),}
+                  @r{it will be expanded using @code{org-store-link-plist}.}
             @r{The sexp must return a string.}
 %<...>      @r{The result of format-time-string on the ... format specification.}
 %t          @r{Timestamp, date only.}
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 1dfffc6..fcaa4f9 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -224,6 +224,8 @@ be replaced with content and expanded in this order:
 
   %[pathname] Insert the contents of the file given by `pathname'.
   %(sexp)     Evaluate elisp `(sexp)' and replace with the result.
+              If sexp's attr is link keyword (%:keyword), it will
+              be expanded using `org-store-link-plist`.
   %<...>      The result of format-time-string on the ... format specification.
   %t          Time stamp, date only.
   %T          Time stamp with date and time.
@@ -1621,10 +1623,26 @@ The template may still contain \"%?\" for cursor positioning."
       (goto-char (match-beginning 0))
       (let ((template-start (point)))
 	(forward-char 1)
-	(let ((result (org-eval (read (current-buffer)))))
+	(let* ((sexp (org-capture-expand-keyword-in-embedded-elisp
+		      (read (current-buffer))))
+	       (result (org-eval sexp)))
 	  (delete-region template-start (point))
 	  (insert result))))))
 
+(defun org-capture--expand-keyword-in-embedded-elisp (attr)
+  "Recursively replace capture link keywords in ATTR sexp.
+Such keywords are prefixed with "%:". See `org-capture-template`
+for more information."
+  (cond ((consp attr)
+	 (mapcar 'org-capture--expand-keyword-in-embedded-elisp attr))
+	((symbolp attr)
+	 (let* ((attr-symbol (symbol-name attr))
+		(key (and (string-match "%\\(:.*\\)" attr-symbol)
+			  (intern (match-string 1 attr-symbol)))))
+	   (or (plist-get org-store-link-plist key)
+	       attr)))
+	(t attr)))
+
 (defun org-capture-inside-embedded-elisp-p ()
   "Return non-nil if point is inside of embedded elisp %(sexp)."
   (let (beg end)
-- 
1.7.9.5

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Nicolas Goaziou]

Ryo TAKAISHI <ryo.takaishi.0@gmail.com> writes:

> I fixed and added document in docstring & org.texi.

Thank you.

> +                  @r{If sexp's attr is link keyword (@code{%:keyword}),}
> +                  @r{it will be expanded using @code{org-store-link-plist}.}

Documentation should not contain a reference to `org-store-link-plist':
it is an internal variable, therefore, it is of no use for the end user.
I suggest something along the lines of:

  Evaluate Elisp SEXP and replace with the results@footnote{For
  convenience, %:keyword (see above) placeholders within the expression
  will be expanded.}. The sexp must return a string.

>    %(sexp)     Evaluate elisp `(sexp)' and replace with the result.
> +              If sexp's attr is link keyword (%:keyword), it will
> +              be expanded using `org-store-link-plist`.

Ditto.

Otherwise the code is fine. Would you provide a complete patch, i.e.
with `git format-patch'? The changelog entry may be:

  org-capture: Expand keywords within %(sexp) placeholder in template

  * org-capture.el (org-capture--expand-keyword-in-embedded-elisp): New
    function.
  (org-capture-expand-embedded-elisp): Use new function.


Regards,

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Ryo TAKAISHI]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Otherwise the code is fine. Would you provide a complete patch, i.e.
> with `git format-patch'? The changelog entry may be:

I create a complete patch for current commit.

Regards,
Ryo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-capture-Expand-keywords-within-sexp-placeholder-.patch --]
[-- Type: text/x-patch, Size: 3194 bytes --]

From d82c99bb643e2c61e1f5b598a687160340a1558f Mon Sep 17 00:00:00 2001
From: Ryo TAKAISHI <ryo.takaishi.0@gmail.com>
Date: Mon, 5 Nov 2012 00:01:08 +0900
Subject: [PATCH] org-capture: Expand keywords within %(sexp) placeholder in
 template

* lisp/org-capture.el: (org-capture--expand-keyword-in-embedded-elisp): New function.
(org-capture-expand-embedded-elisp): Use new function.
---
 doc/org.texi        |    4 +++-
 lisp/org-capture.el |   22 ++++++++++++++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/doc/org.texi b/doc/org.texi
index b23f492..dd1d9b9 100644
--- a/doc/org.texi
+++ b/doc/org.texi
@@ -6852,7 +6852,9 @@ dynamic insertion of content.  The templates are expanded in the order given her
 
 @smallexample
 %[@var{file}]     @r{Insert the contents of the file given by @var{file}.}
-%(@var{sexp})     @r{Evaluate Elisp @var{sexp} and replace with the result.}
+%(@var{sexp})     @r{Evaluate Elisp @var{sexp} and replace with the result@footnote{For }
+                  @r{convenience, %:keyword (see above) placeholders within the expression}
+                  @r{will be expanded.}.}
             @r{The sexp must return a string.}
 %<...>      @r{The result of format-time-string on the ... format specification.}
 %t          @r{Timestamp, date only.}
diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 1dfffc6..d0f7297 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -223,7 +223,9 @@ freely formatted text.  Furthermore, the following %-escapes will
 be replaced with content and expanded in this order:
 
   %[pathname] Insert the contents of the file given by `pathname'.
-  %(sexp)     Evaluate elisp `(sexp)' and replace with the result.
+  %(sexp)     Evaluate elisp `(sexp)' and replace with the result@(For
+              convenience, %:keyword (see above) placeholders within the expression
+              will be expanded.).
   %<...>      The result of format-time-string on the ... format specification.
   %t          Time stamp, date only.
   %T          Time stamp with date and time.
@@ -1621,10 +1623,26 @@ The template may still contain \"%?\" for cursor positioning."
       (goto-char (match-beginning 0))
       (let ((template-start (point)))
 	(forward-char 1)
-	(let ((result (org-eval (read (current-buffer)))))
+	(let* ((sexp (org-capture-expand-keyword-in-embedded-elisp
+		      (read (current-buffer))))
+	       (result (org-eval sexp)))
 	  (delete-region template-start (point))
 	  (insert result))))))
 
+(defun org-capture--expand-keyword-in-embedded-elisp (attr)
+  "Recursively replace capture link keywords in ATTR sexp.
+Such keywords are prefixed with "%:". See `org-capture-template`
+for more information."
+  (cond ((consp attr)
+	 (mapcar 'org-capture--expand-keyword-in-embedded-elisp attr))
+	((symbolp attr)
+	 (let* ((attr-symbol (symbol-name attr))
+		(key (and (string-match "%\\(:.*\\)" attr-symbol)
+			  (intern (match-string 1 attr-symbol)))))
+	   (or (plist-get org-store-link-plist key)
+	       attr)))
+	(t attr)))
+
 (defun org-capture-inside-embedded-elisp-p ()
   "Return non-nil if point is inside of embedded elisp %(sexp)."
   (let (beg end)
-- 
1.7.9.5

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Nicolas Goaziou]

Hello,

Ryo TAKAISHI <ryo.takaishi.0@gmail.com> writes:

> I create a complete patch for current commit.

I've pushed your patch on master (with some documentation tweaks). Thank
you for your work.

Also, please consider signing FSF papers if you want to make other
contributions to Org.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Ryo TAKAISHI]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

Hello,

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Hello,
>
> Ryo TAKAISHI <ryo.takaishi.0@gmail.com> writes:
>
>> I create a complete patch for current commit.
>
> I've pushed your patch on master (with some documentation tweaks). Thank
> you for your work.

Thank you very much.
But, I had forgot to escape double quote in docstring, so attach patch
fixing it.

> Also, please consider signing FSF papers if you want to make other
> contributions to Org.

OK, I want to consider when I send a patch next time.

Regards,
Ryo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-org-capture-Fix-a-docstring.patch --]
[-- Type: text/x-patch, Size: 940 bytes --]

From dacf4ef0677f80cfc589bf5014ca71dfdd2dc3d4 Mon Sep 17 00:00:00 2001
From: Ryo TAKAISHI <ryo.takaishi.0@gmail.com>
Date: Mon, 5 Nov 2012 21:20:14 +0900
Subject: [PATCH] org-capture: Fix a docstring

* lisp/org-capture.el: Fix a docstring.

TINYCHANGE
---
 lisp/org-capture.el |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/org-capture.el b/lisp/org-capture.el
index 695c5eb..ccdb0c1 100644
--- a/lisp/org-capture.el
+++ b/lisp/org-capture.el
@@ -1631,7 +1631,7 @@ The template may still contain \"%?\" for cursor positioning."
 
 (defun org-capture--expand-keyword-in-embedded-elisp (attr)
   "Recursively replace capture link keywords in ATTR sexp.
-Such keywords are prefixed with "%:".  See `org-capture-template'
+Such keywords are prefixed with '%:'.  See `org-capture-template'
 for more information."
   (cond ((consp attr)
 	 (mapcar 'org-capture--expand-keyword-in-embedded-elisp attr))
-- 
1.7.9.5

Re: [PATCH] Capture: Expand keyword within %(SEXP) in template

[Nicolas Goaziou]

Hello,

Ryo TAKAISHI <ryo.takaishi.0@gmail.com> writes:

> But, I had forgot to escape double quote in docstring, so attach patch
> fixing it.

Fixed. Thank you.


Regards,

-- 
Nicolas Goaziou