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" (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 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 > wrote: > >> Derek Chen-Becker 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" >> > + (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 . >> Support Org development at , >> or support my work at >> > > > -- > +---------------------------------------------------------------+ > | 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 | +---------------------------------------------------------------+