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 <yantar92@posteo.net> wrote:

Derek Chen-Becker <derek@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  |

+---------------------------------------------------------------+