MT <[email protected]> writes: > I just finished polishing my code, splitting them into commits, as well as > adding/updating a bunch of test cases! Please check the attached for the > patch files.
Thanks! I will provide some initial comments. > Subject: [PATCH 1/6] lisp/ob-core.el: Fix incorrect interposition of Noweb > prefixes testing/lisp/test-ob.el: Add test. Just "ob-core: Fix incorrect interposition of noweb prefixes" will be enough. No need to put multiple changelog entries all into commit summary. Just put in the commit body as "* testing/lisp/tests-ob.el: Add test." Same in other patches. You can refer to https://orgmode.org/worg/org-contribute.html#commit-messages for more details about commit message format. > In this patch, the empty line is only added when needed (i.e., when > Noweb comments are desired, and the block body does not end with a Noweb > reference), and is done after the interposition. Could you please explain why we do not want to add empty line in all cases (after interposing the prefix)? What is the logic behind "block body does not end with noweb reference"? > diff --git a/lisp/ob-core.el b/lisp/ob-core.el > index 32177fb56..c0ff8659c 100644 > + ... > +(defun org-babel-tangle--unbracketed-link (params) > + "Get a raw link to the src block at point, without brackets. > + > +The PARAMS are the 3rd element of the info for the same src block." > + (unless (string= "no" (cdr (assq :comments params))) > ... > + (if (and org-babel-tangle-use-relative-file-links > + (string-match org-link-types-re bare) > + (string= (match-string 1 bare) "file")) > + (concat "file:" > + (file-relative-name (substring bare (match-end 0)) > + (file-name-directory > + (cdr (assq :tangle params))))) Note that you are using a function from different library that is designed for tangling, not for noweb expansion. As a result, it makes a number of assumptions: 1. It looks into :comments parameter of src block 2. It looks into org-babel-tangle-use-relative-file-links that has nothing to do with noweb expansion 3. It expects :tangle parameter to be non-nil. > ... > + ;; If the block is unnamed, generate one. > + (when (and comment (not (nth 4 info))) > + (setf (nth 4 info) > + (org-babel--generate-block-name > + (org-babel-tangle--unbracketed-link > + (nth 2 info)) > + counter))) This call will err when :tangle is something like "no". This call will also yield unexpected results when INFO has :comments no (as opposed to the parent src block where we are calculating expansions) Finally, this call will generate names depending on org-babel-use-relative-file-links, which is unexpected. You should probably use a different function to generate the block name. Re-using internal function from ob-tangle is not reliable. I will skip reviewing new features for now and focus on bug fixes you submitted. -- 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>
