Nicolas Goaziou wrote: > Hello, > > Nicolas Berthier writes: > >> In any case, as advised earlier in the discussion, I updated one patch >> to integrate new tests for the new feature. I also slightly simplified >> the HTML export patch to avoid useless display attributes. > > Thank you for this nice patch. Some comments follow. > >> + "\\(?:^\\|[^-[:alnum:]]\\)\\(src_\\([^ \f\t\n\r\v\\[]+\\)" > > I think "[^ \f\t\n\r\v[]" is enough. Indeed. I fixed it in the new attached patch. Thanks. As for the definition of what enters into a language name, I suggest leaving it for a subsequent modification as it's not the intended goal of this patch. >> +(defcustom org-babel-exp-inline-code-template >> + "src_%lang[%switches%flags]{%body}" >> + "Template used to export the body of inline code blocks. >> +This template may be customized to include additional information >> +such as the code block name, or the values of particular header >> +arguments. The template is filled out using `org-fill-template', >> +and the following %keys may be used. >> + >> + lang ------ the language of the code block >> + name ------ the name of the code block >> + body ------ the body of the code block >> + switches -- the switches associated to the code block >> + flags ----- the flags passed to the code block >> + >> +In addition to the keys mentioned above, every header argument >> +defined for the code block may be used as a key and will be >> +replaced with its value." >> + :group 'org-babel >> + :type 'string) > > You need to add :version and :package-version values for new defcustoms. Fixed with what I assume are the correct version numbers. Correct me if they are wrong. >> +(ert-deftest ob-exp/exports-inline-code () >> + (should >> + (string-match >> + (replace-regexp-in-string >> + "\\\\\\[]{" "\\(?:\\[]\\)?{" ;accept both src_sh[]{...} or src_sh{...} >> + (regexp-quote "Here is one in the middle src_sh[]{echo 1} of a line. >> +Here is one at the end of a line. src_sh[]{echo 2} >> +src_sh[]{echo 3} Here is one at the beginning of a line. >> +Here is one that is also evaluated: src_sh[]{echo 4} =4=") >> + nil t) >> + (org-test-at-id "cd54fc88-1b6b-45b6-8511-4d8fa7fc8076" >> + (org-narrow-to-subtree) >> + (org-test-with-expanded-babel-code (buffer-string)))))) > > It is a matter of taste, but I think tests should be self-contained. In > particular, it isn't fun debugging `org-test-at-id'. The same goes for > other tests. New tests in "testing/lisp/test-ob.el" are akin already existing tests in this file that were easy to adapt. Considering that the patch adds mostly export-related plus minor syntactic stuff, I think testing babel evaluation itself is not as critical as testing the babel export functionality w.r.t the inline code export, and the org-element parser w.r.t the new syntax. So, I did not add self-contained tests in "test-ob.el" itself, and concentrated on "test-ob-exp.el" and "test-org-element.el". Then, the new patch adds some more self-contained tests to ease debugging simple cases, as well as basic tests for the inline source block parser in `test-org-element/inline-src-block-parser'. Besides, adding new tests made me encounter two unexpected results during export: - First, I discovered that the data resulting from calls to `org-babel-parse-header-arguments' were malformed when it was called with strings starting with a single space (e.g., " :foo bar") as it led to `(intern "")' calls in this function (leading to errors in some other place). For now, I fixed it in the first patch by further extending `org-babel-inline-src-block-regexp' to wipe out all spaces before the headers matching group (as `org-babel-src-block-regexp' does), even though handling cases where `arg' equals "" in `org-babel-parse-header-arguments' may be a cleaner solution; - Secondly, wrapping of results when exporting code with ":results code" switches (as in "src_emacs-lisp[:exports results :results code]{(+ 1 1)}") led to unexpected results. I doubt writing such kind of thing is actually useful, but it does not seem to cost too much to handle simple cases anyway. I added two new failing tests to exhibit such cases in the first patch, and attached a third patch fixing this problem. However, I am not even sure about the results that ought to be expected from the export of such code blocks, and it is still easy to find code blocks that break it (if only by writing code printing newlines). All tests pass after each patch. >> + (if lang >> + (format "%s" lang label code) >> + (format "\n%s" label code))))) > > LANG cannot be nil. So, > > (format "%s" lang label code) > > is sufficient. Done. Thanks.