Before I submit the updated patch, does this look like a better approach?
In the test for `org-base-buffer-file-name' I changed it to explicitly find
the current buffer file name and use that:
(ert-deftest test-org-base-buffer-file-name ()
"Test `org-base-buffer-file-name'."
(should
(org-test-with-temp-text-in-file
"File"
(let ((base-filename (buffer-file-name)))
(and
;; Confirm that we get the same answer whether we provide the buffer
or use the default
(equal base-filename (org-base-buffer-file-name))
(equal base-filename (org-base-buffer-file-name
(current-buffer)))))))
(should
(org-test-with-temp-text-in-file
"File with capture buffer"
(let ((org-capture-templates '(("t" "Test" entry (here) "* Test
Header\n\n")))
(base-filename (buffer-file-name)))
(org-capture nil "t")
(and
;; Confirm that we get the same answer whether we provide the buffer
or use the default
(equal base-filename (org-base-buffer-file-name))
(equal base-filename (org-base-buffer-file-name (current-buffer)))
(equal base-filename (org-base-buffer-file-name (buffer-base-buffer
(current-buffer))))
))))
(should-not
(org-test-with-temp-text
"Buffer without file"
(org-base-buffer-file-name))))
For the tangle test, I also capture the base buffer filename, use an
explicit tangle filename, and ensure that the call to `org-babel-tangle'
returns the expected list of tangled files:
;; See https://list.orgmode.org/87msfxd81c.fsf@localhost/T/#t
(ert-deftest ob-tangle/tangle-from-capture-buffer ()
"Test tangling of source blocks from within a capture buffer.
This is to ensure that we properly resolve the buffer name."
(org-test-with-temp-text-in-file
"* Header\n\nCapture after this point:\n<point>"
(let ((tangle-filename (format "%s.el" (buffer-file-name))))
(should
(unwind-protect
(progn
(let ((org-capture-templates '(("t" "Test" entry (here) "* Test
Header\n\n"))))
(org-capture nil "t")
(goto-char (point-max))
(insert
(format "
#+begin_src elisp :tangle \"%s\" :comments org
(message \"FOO\")
#+end_src" tangle-filename))
(search-backward "message")
;; Confirm that we tangled to the right file
(equal (org-babel-tangle 4) (list tangle-filename))))
;; Clean up the tangled file with the filename from
org-test-with-temp-text-in-file
(delete-file tangle-filename))))))
On Tue, Jan 14, 2025 at 12:26 PM Derek Chen-Becker <de...@chen-becker.org>
wrote:
> Thanks for the comments! This is my first real foray into non-trivial
> elisp so I really appreciate your patience and help. Let me try and get
> these addressed.
>
> Cheers,
>
> derek
>
> On Tue, Jan 14, 2025 at 10:54 AM Ihor Radchenko <yanta...@posteo.net>
> wrote:
>
>> Derek Chen-Becker <de...@chen-becker.org> writes:
>>
>> > Thanks for the help on this! I've reworked the changes into two patches
>> > (one for the README and one for tangle) and I think I've covered your
>> > concerns. I also added a unit test for the org-base-buffer-file-name
>> > function to cover the miss on provided buffers. Please let me know if
>> you
>> > have any questions.
>>
>> Thanks!
>> Although, I am afraid that the second set of unit tests is a bit much
>> - they no longer fit within TINYCHANGE (15-20 significant LOC).
>> We have a legal limit on how much code we can accept without requiring
>> copyright assignment. See
>> https://orgmode.org/worg/org-contribute.html#copyright
>>
>> More comments below.
>>
>> > * lisp/ob-tangle.el (org-babel-tangle): Update to utilize the new
>> > org-base-buffer-file-name function.
>>
>> Our convention is to quote Elisp symbols in commit messages like `this'.
>>
>> > The previous use of buffer-file-name would fail inside of a capture
>> buffer
>> > because it is indirect and does not have an associated filename. This
>>
>> Another convention is double space between sentences.
>>
>> > +(defun org-base-buffer-file-name (&optional BUFFER)
>>
>> Elisp convention is to use lower case for function arguments.
>> Upper case in the docstring is used to highlight the function arguments;
>> it does not mean that the arguments themselves need to be in upper case.
>>
>> > + (if (buffer-base-buffer BUFFER)
>> > + (buffer-file-name (buffer-base-buffer BUFFER))
>> > + (buffer-file-name BUFFER)))
>>
>> Nit: Can also use `if-let*' to avoid calling `buffer-base-buffer' twice.
>>
>> > +;; See https://list.orgmode.org/87msfxd81c.fsf@localhost/T/#t
>> > +(ert-deftest ob-tangle/tangle-from-capture-buffer ()
>> > + "Test tangling of source blocks from within a capture buffer.
>> > +This is to ensure that we properly resolve the buffer name."
>> > + (org-test-with-temp-text-in-file
>> > + "* Header\n\nCapture after this point:\n<point>"
>> > + (should
>> > + (unwind-protect
>> > + (progn
>> > + (let ((org-capture-templates '(("t" "Test" entry (here) "*
>> Test Header\n\n"))))
>> > + (org-capture nil "t")
>> > + (goto-char (point-max))
>> > + (insert "
>> > +#+begin_src elisp :tangle yes :comments org
>> > + (message \"FOO\")
>> > +#+end_src")
>> > + (search-backward "message")
>> > + (org-babel-tangle 4)))
>> > + ;; Clean up the tangled file with the filename from
>> org-test-with-temp-text-in-file
>> > + (delete-file (format "%s.el" file))))))
>>
>> This is not the best style.
>> While technically `file' variable will be let-bound within the
>> `org-test-with-temp-text-in-file' body, it is an internal implementation
>> detail that we should not rely upon. Instead, it is a good idea to
>> examine `buffer-file-name' inside the macro body to get the file name.
>>
>> Also, what if "%s.el" file does not exist? Say, tangling does happen,
>> but to a wrong file. Note that `delete-file' does not throw any error
>> when there is nothing to delete.
>>
>> --
>> Ihor Radchenko // yantar92,
>> Org mode maintainer,
>> 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>
>>
>
>
> --
> +---------------------------------------------------------------+
> | Derek Chen-Becker |
> | GPG Key available at https://keybase.io/dchenbecker and |
> | https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org |
> | Fngrprnt: EB8A 6480 F0A3 C8EB C1E7 7F42 AFC5 AFEE 96E4 6ACC |
> +---------------------------------------------------------------+
>
>
--
+---------------------------------------------------------------+
| Derek Chen-Becker |
| GPG Key available at https://keybase.io/dchenbecker and |
| https://pgp.mit.edu/pks/lookup?search=derek%40chen-becker.org |
| Fngrprnt: EB8A 6480 F0A3 C8EB C1E7 7F42 AFC5 AFEE 96E4 6ACC |
+---------------------------------------------------------------+
