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