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  |
+---------------------------------------------------------------+