[PATCH] Omit file description when :file-desc has nil value

[PATCH] Omit file description when :file-desc has nil value

[Matt Huszagh]

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

Hello,

This patch omits a file description when :file-desc has a nil
value. Previously, the following src block

#+BEGIN_SRC asymptote :results value file :file circle.pdf :file-desc :output-dir img/
  size(2cm);
  draw(unitcircle);
#+END_SRC

would yield

#+RESULTS:
[[file:img/circle.pdf][circle.pdf]]

This makes it impossible (I think) to provide :file-desc with a default
value and prevent the description in some cases. This patch would cause
the same code block to execute to

#+RESULTS:
[[file:img/circle.pdf]]

I feel I may be missing something in regard to why this previously had
the functionality it did. Is there a use case I've missed? To me, the
documentation seems to indicate that my patch is the desired behavior:

   The =file-desc= header argument defines the description (see
   [[*Link Format]]) for the link.  If =file-desc= has no value, the
   "description" part of the link will be omitted.

Full disclaimer: I wrote this section of the documentation as part of
this patch:

https://lists.gnu.org/archive/html/emacs-orgmode/2020-07/msg00320.html

Thanks
Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-core.el-Omit-file-description-when-file-desc.patch --]
[-- Type: text/x-patch, Size: 1303 bytes --]

From edcfa85add6ac71a1e13b7731779ccf4a8e12868 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Wed, 2 Sep 2020 23:06:10 -0700
Subject: [PATCH] lisp/ob-core.el: Omit file description when :file-desc has
 nil value

* lisp/ob-core.el (org-babel-insert-result): Omit file description
when :file-desc value evaluates to nil.

The previous implementation makes it impossible to provide a default
:file-desc and in some cases override it to omit the description.
---
 lisp/ob-core.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 578622232..55165ebc5 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -2257,9 +2257,9 @@ INFO may provide the values of these header arguments (in the
 	 (setq result (org-no-properties result))
 	 (when (member "file" result-params)
 	   (setq result (org-babel-result-to-file
-			 result (when (assq :file-desc (nth 2 info))
-				  (or (cdr (assq :file-desc (nth 2 info)))
-				      result))))))
+			 result (when (and (assq :file-desc (nth 2 info))
+					   (cdr (assq :file-desc (nth 2 info))))
+				  (cdr (assq :file-desc (nth 2 info))))))))
 	((listp result))
 	(t (setq result (format "%S" result))))
   (if (and result-params (member "silent" result-params))
-- 
2.28.0

Re: [PATCH] Omit file description when :file-desc has nil value

[Matt Huszagh]

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

Matt Huszagh <huszaghmatt@gmail.com> writes:

> This patch omits a file description when :file-desc has a nil
> value.

I've modified the patch to yield the same effect when executing a source
block.

Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-core.el-Omit-file-description-when-file-desc.patch --]
[-- Type: text/x-patch, Size: 1771 bytes --]

From 24d156e421973b5a97f1c797d48f1daa95348898 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Wed, 2 Sep 2020 23:06:10 -0700
Subject: [PATCH] lisp/ob-core.el: Omit file description when :file-desc has
 nil value

* lisp/ob-core.el (org-babel-insert-result): Omit file description
when :file-desc value evaluates to nil.
(org-babel-execute-src-block): Perform the same functionality when
executing a src block.

The previous implementation makes it impossible to provide a default
:file-desc and in some cases override it to omit the description.
---
 lisp/ob-core.el | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 578622232..02c0a153c 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -750,7 +750,8 @@ block."
 				    (org-babel-result-to-file
 				     file
 				     (let ((desc (assq :file-desc params)))
-				       (and desc (or (cdr desc) result)))))))
+				       (and (and desc (cdr desc))
+					    (cdr desc)))))))
 		      (setq result (org-babel-ref-resolve post))
 		      (when file
 			(setq result-params (remove "file" result-params))))))
@@ -2257,9 +2258,9 @@ INFO may provide the values of these header arguments (in the
 	 (setq result (org-no-properties result))
 	 (when (member "file" result-params)
 	   (setq result (org-babel-result-to-file
-			 result (when (assq :file-desc (nth 2 info))
-				  (or (cdr (assq :file-desc (nth 2 info)))
-				      result))))))
+			 result (when (and (assq :file-desc (nth 2 info))
+					   (cdr (assq :file-desc (nth 2 info))))
+				  (cdr (assq :file-desc (nth 2 info))))))))
 	((listp result))
 	(t (setq result (format "%S" result))))
   (if (and result-params (member "silent" result-params))
-- 
2.28.0

Re: [PATCH] Omit file description when :file-desc has nil value

[Kyle Meyer]

Matt Huszagh writes:

> Hello,
>
> This patch omits a file description when :file-desc has a nil
> value. Previously, the following src block
>
> #+BEGIN_SRC asymptote :results value file :file circle.pdf :file-desc :output-dir img/
>   size(2cm);
>   draw(unitcircle);
> #+END_SRC
>
> would yield
>
> #+RESULTS:
> [[file:img/circle.pdf][circle.pdf]]
>
> This makes it impossible (I think) to provide :file-desc with a default
> value and prevent the description in some cases.

Hmm, I think that's unfortunately the case.

> I feel I may be missing something in regard to why this previously had
> the functionality it did. Is there a use case I've missed?

A use case was given in the initial patch:
<https://orgmode.org/list/87vclky211.fsf@med.uni-goettingen.de/T/#u>.
The test for this behavior mentioned there is
test-ob/file-desc-header-argument.

That thread links to another thread by gmane ID.  That's this one:
https://orgmode.org/list/87limm4eo2.fsf@med.uni-goettingen.de/T/#u

> To me, the documentation seems to indicate that my patch is the
> desired behavior:
>
>    The =file-desc= header argument defines the description (see
>    [[*Link Format]]) for the link.  If =file-desc= has no value, the
>    "description" part of the link will be omitted.
>
> Full disclaimer: I wrote this section of the documentation as part of
> this patch:
>
> https://lists.gnu.org/archive/html/emacs-orgmode/2020-07/msg00320.html

Right, to reflect the current behavior established as a result of the
above thread, I think that should be reworded to distinguish between an
absent :file-desc header and one with no argument.  Sorry for not
catching that when reviewing your initial patch.

[PATCH] Omit file description when :file-desc has nil value

[Huszaghmatt]

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

     
 

 
 
 
 
 

 
 Kyle Meyer  <kyle@kyleam.com (mailto:kyle@kyleam.com)>  writes:  >  A use case was given in the initial patch:  >   <https://orgmode.org/list/87vclky211.fsf@med.uni-goettingen.de/T/#u>.  >  The test for this behavior mentioned there is  >  test-ob/file-desc-header-argument.  >   >  That thread links to another thread by gmane ID. That's this one:  >   https://orgmode.org/list/87limm4eo2.fsf@med.uni-goettingen.de/T/#u  Thanks for the reply, Kyle, and thanks for pointing me to that thread. I understand that this would break existing functionality, but I think my solution makes more sense. For one, I think that the current implementation is a bit confusing. More importantly though, it makes it impossible to both provide a default value for :file-desc and omit it in some cases. The benefit (as mentioned in that thread) is that in those select cases, the same argument would not need to be provided twice. I think the cost of the current functionality outweighs the benefit. What are your t
houghts? I've got a pending patch (https://lists.gnu.org/archive/html/emacs-orgmode/2020-09/msg00041.html) that allows a user to provide lambdas as default header arguments (evaluated during source block execution or export). This makes the use of defaults much more attractive in my mind because they can provide context aware values. Similarly, it increases the cost of the current implementation because then this facility cannot be used for :file-desc. I guess there are other solutions we could explore, such as an empty string (is an empty file descriptor ever a valid use case?) taking the place of the current functionality, or fully eliminating the file descriptor. However, this is starting to feel like a lot of hacks and would be very confusing to new users. Moreover, it really just pushes the problem down the road rather than fixing it outright.  >  Right, to reflect the current behavior established as a result of the  >  above thread, I think that should be reworded to distinguis
h between an  >  absent :file-desc header and one with no argument. Sorry for not  >  catching that when reviewing your initial patch. No worries, and I agree the documentation should be updated. I'm happy to provide the patch myself, but I'd like to talk through whether the current implementation is the correct one before I do. Best Matt 

 
 
 
 

 
     

[-- Attachment #2: Type: text/html, Size: 3655 bytes --]

Re: [PATCH] Omit file description when :file-desc has nil value

[Kyle Meyer]

Hi Matt,

It looks like this message got detached from the original thread [*] and
ended up a bit misformatted (at least for plain-text readers).  This
seems to be the message you accidentally sent to me off-list, so I will
copy my reply here as well.

  [*] https://orgmode.org/list/87tuwef76g.fsf@kyleam.com

Matt Huszagh writes:

> Thanks for the reply, Kyle, and thanks for pointing me to that thread. I
> understand that this would break existing functionality, but I think my
> solution makes more sense. For one, I think that the current
> implementation is a bit confusing. More importantly though, it makes it
> impossible to both provide a default value for :file-desc and omit it in
> some cases. The benefit (as mentioned in that thread) is that in those
> select cases, the same argument would not need to be provided twice. I
> think the cost of the current functionality outweighs the benefit. What
> are your thoughts?

I also don't find the current behavior particularly intuitive.  (I'm
also not really a babel user, so my opinion probably shouldn't count for
much.)  If we were adding it today, I think what you describe would be
better, but, as you mention, breakage also now also weighs against
making a change here.

In any case, I'd suggest raising the discussion on the list after the
9.4 release.

>> Right, to reflect the current behavior established as a result of the
>> above thread, I think that should be reworded to distinguish between an
>> absent :file-desc header and one with no argument.  Sorry for not
>> catching that when reviewing your initial patch.
>
> No worries, and I agree the documentation should be updated. I'm happy
> to provide the patch myself, but I'd like to talk through whether the
> current implementation is the correct one before I do.

Thanks.  To avoid any confusion coming from this description making it
into the 9.4 release, I've updated it in 4b2123fb7.

Re: [PATCH] Omit file description when :file-desc has nil value

[Matt Huszagh]

Kyle Meyer <kyle@kyleam.com> writes:

> I also don't find the current behavior particularly intuitive.  (I'm
> also not really a babel user, so my opinion probably shouldn't count for
> much.)  If we were adding it today, I think what you describe would be
> better, but, as you mention, breakage also now also weighs against
> making a change here.
>
> In any case, I'd suggest raising the discussion on the list after the
> 9.4 release.

Ok, I'll follow up on this then.

>>> Right, to reflect the current behavior established as a result of the
>>> above thread, I think that should be reworded to distinguish between an
>>> absent :file-desc header and one with no argument.  Sorry for not
>>> catching that when reviewing your initial patch.
>>
>> No worries, and I agree the documentation should be updated. I'm happy
>> to provide the patch myself, but I'd like to talk through whether the
>> current implementation is the correct one before I do.
>
> Thanks.  To avoid any confusion coming from this description making it
> into the 9.4 release, I've updated it in 4b2123fb7.

Thanks for fixing that Kyle.

Matt

Re: [PATCH] Omit file description when :file-desc has nil value

[Matt Huszagh]

> Kyle Meyer <kyle@kyleam.com> writes:
>
>> I also don't find the current behavior particularly intuitive.  (I'm
>> also not really a babel user, so my opinion probably shouldn't count for
>> much.)  If we were adding it today, I think what you describe would be
>> better, but, as you mention, breakage also now also weighs against
>> making a change here.
>>
>> In any case, I'd suggest raising the discussion on the list after the
>> 9.4 release.

Hello, just following up on this since 9.4 has been released. Thoughts?

Matt

Re: [PATCH] Omit file description when :file-desc has nil value

[Kyle Meyer]

Matt Huszagh writes:

>> Kyle Meyer <kyle@kyleam.com> writes:
>>
>>> I also don't find the current behavior particularly intuitive.  (I'm
>>> also not really a babel user, so my opinion probably shouldn't count for
>>> much.)  If we were adding it today, I think what you describe would be
>>> better, but, as you mention, breakage also now also weighs against
>>> making a change here.
>>>
>>> In any case, I'd suggest raising the discussion on the list after the
>>> 9.4 release.
>
> Hello, just following up on this since 9.4 has been released. Thoughts?

No babel users have chimed in.

My current opinion is that I'd prefer not to break the use case
mentioned earlier in this discussion [1].  It may not be intuitive, but
it's longstanding and I don't have a sense for how much it's relied on.

  [1] https://orgmode.org/list/87tuwef76g.fsf@kyleam.com/
      https://orgmode.org/list/87limm4eo2.fsf@med.uni-goettingen.de/T/#u

Quoting what you said earlier:

> For one, I think that the current implementation is a bit
> confusing. More importantly though, it makes it impossible to both
> provide a default value for :file-desc and omit it in some cases. The
> benefit (as mentioned in that thread) is that in those select cases,
> the same argument would not need to be provided twice. I think the
> cost of the current functionality outweighs the benefit.

But it's not a direct comparison against that use case and the use case
you want to support.  The potential breakage of existing documents is a
big factor to go against.

> I guess there are other solutions we could explore, such as an empty
> string (is an empty file descriptor ever a valid use case?) taking the
> place of the current functionality, or fully eliminating the file
> descriptor. However, this is starting to feel like a lot of hacks and
> would be very confusing to new users.

Unfortunately, such a kludge is how I'd suggest to move forward.
Perhaps an empty string, or perhaps any value (e.g., ":file-desc []")
that org-babel-read won't treat as a string or nil (the two cases that
mean something right now).  The rough patch below is an example of the
latter.

> Moreover, it really just pushes the problem down the road rather than
> fixing it outright.

I'm not sure I get this.  What's next down the road in this scenario?
With something like the above kludge, haven't we exhausted the cases for
:file-desc?


diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 7300f239e..4483585a1 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -646,6 +646,13 @@ (defun org-babel--expand-body (info)
       (replace-regexp-in-string
        (org-src-coderef-regexp coderef) "" expand nil nil 1))))
 
+(defun org-babel--file-desc (params result)
+  (pcase (assq :file-desc params)
+    (`nil nil)
+    (`(:file-desc) result)
+    (`(:file-desc . ,(and (pred stringp) val)) val)
+    (_ nil)))
+
 ;;;###autoload
 (defun org-babel-execute-src-block (&optional arg info params)
   "Execute the current source code block.
@@ -749,8 +756,7 @@ (defun org-babel-execute-src-block (&optional arg info params)
 		    (let ((*this* (if (not file) result
 				    (org-babel-result-to-file
 				     file
-				     (let ((desc (assq :file-desc params)))
-				       (and desc (or (cdr desc) result)))))))
+				     (org-babel--file-desc params result)))))
 		      (setq result (org-babel-ref-resolve post))
 		      (when file
 			(setq result-params (remove "file" result-params))))))
@@ -2257,9 +2263,8 @@ (defun org-babel-insert-result (result &optional result-params info hash lang)
 	 (setq result (org-no-properties result))
 	 (when (member "file" result-params)
 	   (setq result (org-babel-result-to-file
-			 result (when (assq :file-desc (nth 2 info))
-				  (or (cdr (assq :file-desc (nth 2 info)))
-				      result))))))
+			 result
+			 (org-babel--file-desc (nth 2 info) result)))))
 	((listp result))
 	(t (setq result (format "%S" result))))
   (if (and result-params (member "silent" result-params))
diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
index 648e9c115..e7a292de3 100644
--- a/testing/lisp/test-ob.el
+++ b/testing/lisp/test-ob.el
@@ -1084,7 +1084,16 @@ (ert-deftest test-ob/file-desc-header-argument ()
     (org-babel-execute-src-block)
     (goto-char (point-min))
     (should (search-forward "[[file:foo][bar]]" nil t))
-    (should (search-forward "[[file:foo][foo]]" nil t))))
+    (should (search-forward "[[file:foo][foo]]" nil t)))
+  (should (string-match-p
+	   (regexp-quote "[[file:foo]]")
+	   (org-test-with-temp-text "
+#+begin_src emacs-lisp :results file :file-desc []
+  \"foo\"
+#+end_src"
+	     (org-babel-next-src-block)
+	     (org-babel-execute-src-block)
+	     (buffer-substring-no-properties (point-min) (point-max))))))
 
 (ert-deftest test-ob/result-file-link-type-header-argument ()
   "Ensure that the result is a link to a file.

Re: [PATCH] Omit file description when :file-desc has nil value

[Matt Huszagh]

Kyle Meyer <kyle@kyleam.com> writes:

> But it's not a direct comparison against that use case and the use case
> you want to support.  The potential breakage of existing documents is a
> big factor to go against.

Yep, I agree. I think my phrasing could have just been better. I meant
to include the breakage as a factor against.

> Unfortunately, such a kludge is how I'd suggest to move forward.
> Perhaps an empty string, or perhaps any value (e.g., ":file-desc []")
> that org-babel-read won't treat as a string or nil (the two cases that
> mean something right now).  The rough patch below is an example of the
> latter.

I like this solution better than mine. I guess it's still a bit of a
hack, but it doesn't seem to be one that could break a use case, whereas
the empty string could conceivably be intended, though I'm still not
sure why.

> I'm not sure I get this.  What's next down the road in this scenario?
> With something like the above kludge, haven't we exhausted the cases for
> :file-desc?

Yes I think you're right. I was referring to my solution of an empty
string, which I didn't see a personal use for, but felt it might be a
valid use case for someone else. I really can't think of any reason the
empty vector would otherwise be valid.

> diff --git a/lisp/ob-core.el b/lisp/ob-core.el
> index 7300f239e..4483585a1 100644
> --- a/lisp/ob-core.el
> +++ b/lisp/ob-core.el
> @@ -646,6 +646,13 @@ (defun org-babel--expand-body (info)
>        (replace-regexp-in-string
>         (org-src-coderef-regexp coderef) "" expand nil nil 1))))
>  
> +(defun org-babel--file-desc (params result)
> +  (pcase (assq :file-desc params)
> +    (`nil nil)
> +    (`(:file-desc) result)
> +    (`(:file-desc . ,(and (pred stringp) val)) val)
> +    (_ nil)))
> +
>  ;;;###autoload
>  (defun org-babel-execute-src-block (&optional arg info params)
>    "Execute the current source code block.
> @@ -749,8 +756,7 @@ (defun org-babel-execute-src-block (&optional arg info params)
>  		    (let ((*this* (if (not file) result
>  				    (org-babel-result-to-file
>  				     file
> -				     (let ((desc (assq :file-desc params)))
> -				       (and desc (or (cdr desc) result)))))))
> +				     (org-babel--file-desc params result)))))
>  		      (setq result (org-babel-ref-resolve post))
>  		      (when file
>  			(setq result-params (remove "file" result-params))))))
> @@ -2257,9 +2263,8 @@ (defun org-babel-insert-result (result &optional result-params info hash lang)
>  	 (setq result (org-no-properties result))
>  	 (when (member "file" result-params)
>  	   (setq result (org-babel-result-to-file
> -			 result (when (assq :file-desc (nth 2 info))
> -				  (or (cdr (assq :file-desc (nth 2 info)))
> -				      result))))))
> +			 result
> +			 (org-babel--file-desc (nth 2 info) result)))))
>  	((listp result))
>  	(t (setq result (format "%S" result))))
>    (if (and result-params (member "silent" result-params))
> diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
> index 648e9c115..e7a292de3 100644
> --- a/testing/lisp/test-ob.el
> +++ b/testing/lisp/test-ob.el
> @@ -1084,7 +1084,16 @@ (ert-deftest test-ob/file-desc-header-argument ()
>      (org-babel-execute-src-block)
>      (goto-char (point-min))
>      (should (search-forward "[[file:foo][bar]]" nil t))
> -    (should (search-forward "[[file:foo][foo]]" nil t))))
> +    (should (search-forward "[[file:foo][foo]]" nil t)))
> +  (should (string-match-p
> +	   (regexp-quote "[[file:foo]]")
> +	   (org-test-with-temp-text "
> +#+begin_src emacs-lisp :results file :file-desc []
> +  \"foo\"
> +#+end_src"
> +	     (org-babel-next-src-block)
> +	     (org-babel-execute-src-block)
> +	     (buffer-substring-no-properties (point-min) (point-max))))))
>  
>  (ert-deftest test-ob/result-file-link-type-header-argument ()
>    "Ensure that the result is a link to a file.

This patch looks good. I've tested it and it works well for me. Thanks
for coming up with a good solution! I think the one thing still missing
is some documentation in the info manual. Something along the lines of

     The ‘file-desc’ header argument defines the description (see *note
     Link Format::) for the link.  If ‘file-desc’ has no value, Org uses
     the generated file name for both the “link” and “description” parts
     of the link. If you want to omit the description (i.e., [[link]]),
     you can either omit the ‘file-desc’ header argument or provide it
     with an empty vector (i.e., :file-desc []).

Feel free to add this (or something else) to your patch. Or, if you'd
prefer that I created a patch for it, let me know.

Matt

Re: [PATCH] Omit file description when :file-desc has nil value

[Kyle Meyer]

Matt Huszagh writes:

> This patch looks good. I've tested it and it works well for me. Thanks
> for coming up with a good solution!

Thanks for testing it out.

> I think the one thing still missing is some documentation in the info
> manual. Something along the lines of [...]

Yep, the manual should definitely be updated in a final patch.  What you
suggest looks good to me.

> Feel free to add this (or something else) to your patch. Or, if you'd
> prefer that I created a patch for it, let me know.

I'd be happy for you to take what I sent and work it into a proper
patch.  Here are some other loose ends in addition to the manual update
you mentioned:

  * a NEWS entry for 9.5

  * decide whether (:file-desc . []) should be handled explicitly rather
    than the current "any value that org-babel-read doesn't map to nil
    or a string"

  * check that there's a test case for each :file-desc scenario

Thanks.

Re: [PATCH] Omit file description when :file-desc has nil value

[Matt Huszagh]

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

Kyle Meyer <kyle@kyleam.com> writes:

> I'd be happy for you to take what I sent and work it into a proper
> patch.  Here are some other loose ends in addition to the manual update
> you mentioned:
>
>   * a NEWS entry for 9.5
>
>   * decide whether (:file-desc . []) should be handled explicitly rather
>     than the current "any value that org-babel-read doesn't map to nil
>     or a string"
>
>   * check that there's a test case for each :file-desc scenario

Let me know if I missed anything, or any other issues. I've decided to
handle the empty vector case explicitly. I think this behavior is
clearer.

Thanks,
Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-list-ob-core.el-Allow-passing-empty-vector-to-file-d.patch --]
[-- Type: text/x-patch, Size: 6005 bytes --]

From 749fd5ade6b65f9d07e87b4af44ebb1afef2bee6 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Tue, 29 Sep 2020 14:11:59 -0700
Subject: [PATCH] list/ob-core.el: Allow passing empty vector to :file-desc to
 omit description

* doc/org-manual.org (Type): Document empty vector argument for
file-desc.
* etc/ORG-NEWS (New argument for ~file-desc~ babel header): Add entry
to NEWS.
* lisp/ob-core.el (org-babel--file-desc): Add new function to evaluate
file description value.
(org-babel-execute-src-block): Correctly evaluate file description
when executing src block.
(org-babel-insert-result): Correctly evaluate file description value
when inserting the result of src block execution into the buffer.
* testing/lisp/test-ob.el (test-ob/file-desc-header-argument): Add
test case for new behavior.
---
 doc/org-manual.org      |  8 +++++---
 etc/ORG-NEWS            | 13 ++++++++++++-
 lisp/ob-core.el         | 16 +++++++++++-----
 testing/lisp/test-ob.el | 20 +++++++++++++++++++-
 4 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/doc/org-manual.org b/doc/org-manual.org
index e7d25b90e..a790f3225 100644
--- a/doc/org-manual.org
+++ b/doc/org-manual.org
@@ -17482,10 +17482,12 @@ default behavior is to automatically determine the result type.
   #+end_example
 
   #+cindex: @samp{file-desc}, header argument
-  The =file-desc= header argument defines the description (see
-  [[*Link Format]]) for the link.  If =file-desc= is present but has no value,
+  The =file-desc= header argument defines the description (see [[*Link
+  Format]]) for the link.  If =file-desc= is present but has no value,
   the =file= value is used as the link description.  When this
-  argument is not present, the description is omitted.
+  argument is not present, the description is omitted.  If you want to
+  provide the =file-disc= argument but omit the description, you can
+  provide it with an empty vector (i.e., :file-desc []).
 
   #+cindex: @samp{sep}, header argument
   By default, Org assumes that a table written to a file has
diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 5dc68cba4..19f6af288 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -24,8 +24,19 @@ Earlier, IDs generated using =ts= method had a hard-coded format (i.e. =20200923
 The new option allows user to customise the format.
 Defaults are unchanged.
 
+*** New argument for ~file-desc~ babel header
+
+It is now possible to provide the =file-desc= header argument for a
+babel source block but omit the description by passing an empty vector
+as an argument (i.e., :file-desc []).  This can be useful because
+providing =file-desc= without an argument results in the result of
+=file= being used in the description.  Previously, the only way to
+omit a file description was to omit the header argument entirely,
+which made it difficult/impossible to provide a default value for
+=file-desc=.
+
 ** New features
-*** =ob-python= improvements to =:return= header argument 
+*** =ob-python= improvements to =:return= header argument
 
 The =:return= header argument in =ob-python= now works for session
 blocks as well as non-session blocks.  Also, it now works with the
diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 7300f239e..075e3f928 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -646,6 +646,14 @@ a list with the following pattern:
       (replace-regexp-in-string
        (org-src-coderef-regexp coderef) "" expand nil nil 1))))
 
+(defun org-babel--file-desc (params result)
+  "Retrieve file description."
+  (pcase (assq :file-desc params)
+    (`nil nil)
+    (`(:file-desc) result)
+    (`(:file-desc . ,(and (pred stringp) val)) val)
+    (`(:file-desc . []) nil)))
+
 ;;;###autoload
 (defun org-babel-execute-src-block (&optional arg info params)
   "Execute the current source code block.
@@ -749,8 +757,7 @@ block."
 		    (let ((*this* (if (not file) result
 				    (org-babel-result-to-file
 				     file
-				     (let ((desc (assq :file-desc params)))
-				       (and desc (or (cdr desc) result)))))))
+				     (org-babel--file-desc params result)))))
 		      (setq result (org-babel-ref-resolve post))
 		      (when file
 			(setq result-params (remove "file" result-params))))))
@@ -2257,9 +2264,8 @@ INFO may provide the values of these header arguments (in the
 	 (setq result (org-no-properties result))
 	 (when (member "file" result-params)
 	   (setq result (org-babel-result-to-file
-			 result (when (assq :file-desc (nth 2 info))
-				  (or (cdr (assq :file-desc (nth 2 info)))
-				      result))))))
+			 result
+			 (org-babel--file-desc (nth 2 info) result)))))
 	((listp result))
 	(t (setq result (format "%S" result))))
   (if (and result-params (member "silent" result-params))
diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
index 648e9c115..df4b13498 100644
--- a/testing/lisp/test-ob.el
+++ b/testing/lisp/test-ob.el
@@ -1084,7 +1084,25 @@ trying to find the :END: marker."
     (org-babel-execute-src-block)
     (goto-char (point-min))
     (should (search-forward "[[file:foo][bar]]" nil t))
-    (should (search-forward "[[file:foo][foo]]" nil t))))
+    (should (search-forward "[[file:foo][foo]]" nil t)))
+  (should (string-match-p
+           (regexp-quote "[[file:foo]]")
+           (org-test-with-temp-text "
+#+begin_src emacs-lisp :results file :file-desc []
+  \"foo\"
+#+end_src"
+             (org-babel-next-src-block)
+             (org-babel-execute-src-block)
+             (buffer-substring-no-properties (point-min) (point-max)))))
+  (should (string-match-p
+           (regexp-quote "[[file:foo][foo]]")
+           (org-test-with-temp-text "
+#+begin_src emacs-lisp :results file :file-desc
+  \"foo\"
+#+end_src"
+             (org-babel-next-src-block)
+             (org-babel-execute-src-block)
+             (buffer-substring-no-properties (point-min) (point-max))))))
 
 (ert-deftest test-ob/result-file-link-type-header-argument ()
   "Ensure that the result is a link to a file.
-- 
2.28.0

Re: [PATCH] Omit file description when :file-desc has nil value

[Kyle Meyer]

Matt Huszagh writes:

> Subject: [PATCH] list/ob-core.el: Allow passing empty vector to :file-desc to
>  omit description

s/list/lisp/

> diff --git a/doc/org-manual.org b/doc/org-manual.org
> index e7d25b90e..a790f3225 100644
> --- a/doc/org-manual.org
> +++ b/doc/org-manual.org
> @@ -17482,10 +17482,12 @@ default behavior is to automatically determine the result type.
>    #+end_example
>  
>    #+cindex: @samp{file-desc}, header argument
> -  The =file-desc= header argument defines the description (see
> -  [[*Link Format]]) for the link.  If =file-desc= is present but has no value,
> +  The =file-desc= header argument defines the description (see [[*Link
> +  Format]]) for the link.  If =file-desc= is present but has no value,
>    the =file= value is used as the link description.  When this
> -  argument is not present, the description is omitted.
> +  argument is not present, the description is omitted.  If you want to
> +  provide the =file-disc= argument but omit the description, you can

s/file-disc/file-desc/

> +  provide it with an empty vector (i.e., :file-desc []).

> diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
> index 5dc68cba4..19f6af288 100644
> --- a/etc/ORG-NEWS
> +++ b/etc/ORG-NEWS
> @@ -24,8 +24,19 @@ Earlier, IDs generated using =ts= method had a hard-coded format (i.e. =20200923
>  The new option allows user to customise the format.
>  Defaults are unchanged.
>  
> +*** New argument for ~file-desc~ babel header
[...]
>  ** New features
> -*** =ob-python= improvements to =:return= header argument 
> +*** =ob-python= improvements to =:return= header argument

This space change is touching unrelated code.

>  The =:return= header argument in =ob-python= now works for session
>  blocks as well as non-session blocks.  Also, it now works with the
> diff --git a/lisp/ob-core.el b/lisp/ob-core.el
> index 7300f239e..075e3f928 100644
> --- a/lisp/ob-core.el
> +++ b/lisp/ob-core.el
> @@ -646,6 +646,14 @@ a list with the following pattern:
>        (replace-regexp-in-string
>         (org-src-coderef-regexp coderef) "" expand nil nil 1))))
>  
> +(defun org-babel--file-desc (params result)
> +  "Retrieve file description."
> +  (pcase (assq :file-desc params)
> +    (`nil nil)

All right, so this is the no header case...

> +    (`(:file-desc) result)

...this is for when org-babel-read maps the value to nil...

> +    (`(:file-desc . ,(and (pred stringp) val)) val)

...and when org-babel-read maps it to a string...

> +    (`(:file-desc . []) nil)))

...and this the explicit vector.

Operationally any value that org-babel-read doesn't map to nil or a
string leads to nil.  The pcase expression then could be reduced to

    (pcase (assq :file-desc params)
      (`(:file-desc) result)
      (`(:file-desc . ,(and (pred stringp) val)) val))

However, I'm okay with it as is, particularly the [] arm, because I
think it helps readability wise to explicitly spell out the documented
case.

So, with the typo/spurious space change clean-ups, this looks good to
me.  IIRC from a previous thread, you haven't yet completed the
copyright paperwork.  Is that the case?

Thanks.

Re: [PATCH] Omit file description when :file-desc has nil value

[Matt Huszagh]

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

Kyle Meyer <kyle@kyleam.com> writes:

> So, with the typo/spurious space change clean-ups, this looks good to
> me.  IIRC from a previous thread, you haven't yet completed the
> copyright paperwork.  Is that the case?

I've made those fixes and attached the updated patch.

I also sent you the paperwork separately (didn't post to the thread
since the PDF is too large).

Thanks
Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-core.el-Allow-passing-empty-vector-to-file-d.patch --]
[-- Type: text/x-patch, Size: 5807 bytes --]

From 7452f3e8315be63fa8ae160f6be00963bac898a7 Mon Sep 17 00:00:00 2001
From: Matt Huszagh <huszaghmatt@gmail.com>
Date: Tue, 29 Sep 2020 14:11:59 -0700
Subject: [PATCH] lisp/ob-core.el: Allow passing empty vector to :file-desc to
 omit description

* doc/org-manual.org (Type): Document empty vector argument for
file-desc.
* etc/ORG-NEWS (New argument for ~file-desc~ babel header): Add entry
to NEWS.
* lisp/ob-core.el (org-babel--file-desc): Add new function to evaluate
file description value.
(org-babel-execute-src-block): Correctly evaluate file description
when executing src block.
(org-babel-insert-result): Correctly evaluate file description value
when inserting the result of src block execution into the buffer.
* testing/lisp/test-ob.el (test-ob/file-desc-header-argument): Add
test case for new behavior.
---
 doc/org-manual.org      |  8 +++++---
 etc/ORG-NEWS            | 11 +++++++++++
 lisp/ob-core.el         | 16 +++++++++++-----
 testing/lisp/test-ob.el | 20 +++++++++++++++++++-
 4 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/doc/org-manual.org b/doc/org-manual.org
index e7d25b90e..ef2dad9ef 100644
--- a/doc/org-manual.org
+++ b/doc/org-manual.org
@@ -17482,10 +17482,12 @@ default behavior is to automatically determine the result type.
   #+end_example
 
   #+cindex: @samp{file-desc}, header argument
-  The =file-desc= header argument defines the description (see
-  [[*Link Format]]) for the link.  If =file-desc= is present but has no value,
+  The =file-desc= header argument defines the description (see [[*Link
+  Format]]) for the link.  If =file-desc= is present but has no value,
   the =file= value is used as the link description.  When this
-  argument is not present, the description is omitted.
+  argument is not present, the description is omitted.  If you want to
+  provide the =file-desc= argument but omit the description, you can
+  provide it with an empty vector (i.e., :file-desc []).
 
   #+cindex: @samp{sep}, header argument
   By default, Org assumes that a table written to a file has
diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 5dc68cba4..7f935bf52 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -24,6 +24,17 @@ Earlier, IDs generated using =ts= method had a hard-coded format (i.e. =20200923
 The new option allows user to customise the format.
 Defaults are unchanged.
 
+*** New argument for ~file-desc~ babel header
+
+It is now possible to provide the =file-desc= header argument for a
+babel source block but omit the description by passing an empty vector
+as an argument (i.e., :file-desc []).  This can be useful because
+providing =file-desc= without an argument results in the result of
+=file= being used in the description.  Previously, the only way to
+omit a file description was to omit the header argument entirely,
+which made it difficult/impossible to provide a default value for
+=file-desc=.
+
 ** New features
 *** =ob-python= improvements to =:return= header argument 
 
diff --git a/lisp/ob-core.el b/lisp/ob-core.el
index 7300f239e..075e3f928 100644
--- a/lisp/ob-core.el
+++ b/lisp/ob-core.el
@@ -646,6 +646,14 @@ a list with the following pattern:
       (replace-regexp-in-string
        (org-src-coderef-regexp coderef) "" expand nil nil 1))))
 
+(defun org-babel--file-desc (params result)
+  "Retrieve file description."
+  (pcase (assq :file-desc params)
+    (`nil nil)
+    (`(:file-desc) result)
+    (`(:file-desc . ,(and (pred stringp) val)) val)
+    (`(:file-desc . []) nil)))
+
 ;;;###autoload
 (defun org-babel-execute-src-block (&optional arg info params)
   "Execute the current source code block.
@@ -749,8 +757,7 @@ block."
 		    (let ((*this* (if (not file) result
 				    (org-babel-result-to-file
 				     file
-				     (let ((desc (assq :file-desc params)))
-				       (and desc (or (cdr desc) result)))))))
+				     (org-babel--file-desc params result)))))
 		      (setq result (org-babel-ref-resolve post))
 		      (when file
 			(setq result-params (remove "file" result-params))))))
@@ -2257,9 +2264,8 @@ INFO may provide the values of these header arguments (in the
 	 (setq result (org-no-properties result))
 	 (when (member "file" result-params)
 	   (setq result (org-babel-result-to-file
-			 result (when (assq :file-desc (nth 2 info))
-				  (or (cdr (assq :file-desc (nth 2 info)))
-				      result))))))
+			 result
+			 (org-babel--file-desc (nth 2 info) result)))))
 	((listp result))
 	(t (setq result (format "%S" result))))
   (if (and result-params (member "silent" result-params))
diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
index 648e9c115..df4b13498 100644
--- a/testing/lisp/test-ob.el
+++ b/testing/lisp/test-ob.el
@@ -1084,7 +1084,25 @@ trying to find the :END: marker."
     (org-babel-execute-src-block)
     (goto-char (point-min))
     (should (search-forward "[[file:foo][bar]]" nil t))
-    (should (search-forward "[[file:foo][foo]]" nil t))))
+    (should (search-forward "[[file:foo][foo]]" nil t)))
+  (should (string-match-p
+           (regexp-quote "[[file:foo]]")
+           (org-test-with-temp-text "
+#+begin_src emacs-lisp :results file :file-desc []
+  \"foo\"
+#+end_src"
+             (org-babel-next-src-block)
+             (org-babel-execute-src-block)
+             (buffer-substring-no-properties (point-min) (point-max)))))
+  (should (string-match-p
+           (regexp-quote "[[file:foo][foo]]")
+           (org-test-with-temp-text "
+#+begin_src emacs-lisp :results file :file-desc
+  \"foo\"
+#+end_src"
+             (org-babel-next-src-block)
+             (org-babel-execute-src-block)
+             (buffer-substring-no-properties (point-min) (point-max))))))
 
 (ert-deftest test-ob/result-file-link-type-header-argument ()
   "Ensure that the result is a link to a file.
-- 
2.28.0

Re: [PATCH] Omit file description when :file-desc has nil value

[Kyle Meyer]

Matt Huszagh writes:

> I've made those fixes and attached the updated patch.

Applied (d9884cfa7).

Thank you!