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

[Kyle Meyer <kyle@kyleam.com>]

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.