Re: [PATCH] (v2) New LaTeX code export option: engraved

[Timothy <tecosaur@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5007 bytes --]

Hi Ihor,

> Before merging, could you also try to implement tests at least for
> engraved feature? If I recall correctly, we do not currently have
> backend-specific tests. But it would certainly help if we did. You might
> as well write a small “nucleus” test for ox-latex.

Probably not a bad idea, I’m just not sure what/how I’d go about this.

> Also, unless I miss something, there is no support for coderefs in the
> patchset. Is it intentional?

I think you’ve missed something. It has the same coderefs support as minted.

> It appears that the code for caption-str is duplicated.  It could be
> also factored out.

It could, but I’m not sure this is actually a good idea as the duplication is
more the result of chance than a common dependency.

>> +      (format-spec custom-env
>> +                   `((?s . ,formatted-src)
>> +                     (?c . ,caption)
>> +                     (?f . ,float)
>> +                     (?l . ,(org-latex–label src-block info))
>> +                     (?o . ,(or (plist-get attributes :options) “”)))))))
>
> I do not see a definition of `format-spec’ function. There is only
> `spec’ above in the code. Can you double check? Either I am missing
> something or something fishy is going on.

This code isn’t introduced by my patches, just relocated. FWIW `format-spec' is
provided by Emacs, and I don’t think is new.

>> +  (let ((code (org-element-property :value inline-src-block))
>> +        (lang (org-element-property :language inline-src-block)))
>> +    (pcase (plist-get info :latex-listings)
>> +      (’nil (org-latex–text-markup code ’code info))
>> +      (’minted (org-latex-inline-src-block–minted info code lang))
>> +      (_ (org-latex-inline-src-block–listings info code lang)))))
>
> Please use `nil and `minted.  Using ’ may create issues in older Emacs.

Done.

>> +% In case engrave-faces-latex-gen-preamble has not been run.
>> +\
>> +\
>
> What is the difference between EfD and EFD? EfD is also not documented.

Documentation added. EfD is the background colour.

>> +FVEXTRA-SETUP sets fvextra’s defaults according to
>> +`org-latex-engraved-options’, and LISTINGS-SETUP creates the
>> +listings environment and defines \.“
>
> It is unclear what listings environment does given that we use engraved
> package. Can you provide a bit more details in the docstring on what the
> user will get if [LISTINGS-SETUP] is present.
> It may catch users by surprise that deleting this will make captions
> disappear.

It does the same thing as minted’s `listings' environment, hence the choice of
name. Documentation added.

> Why not just
> (org-element-map (plist-get info :parse-tree)
>                         ’(src-block inline-src-block example-block fixed-width) #’identity
>                         info t)

Ah, in my config I’m also using some engraved info for example/fixed-width
blocks. I’ll leave this out for now.

>>      (pcase (plist-get info :latex-listings)
>>        (’nil (org-latex–text-markup code ’code info))
>>        …
> Same comment about ` in pcase.

Done.

> What will happen if there is no [LISTINGS-SETUP]?

Captioned/Floating code blocks won’t work.

>> +(defcustom org-latex-engraved-theme nil
> Docstring does not explain what happens when set to nil.
> Also, does :type ’symbol allow t and nil?

I think so, `symbolp' says they’re symbols if nothing else.

> Will it work when engraved-theme is t?

Yes.

> What about example-block and fixed-width? I may be missing something,
> but earlier in the patchset you had conditionals of the type
> (or src-p fixedw-p). So, does engrave-faces do anything with fixedw-type
> elements?

Resolved earlier.

>> +         (gen-theme-spec
>> +          (lambda (theme) …
>
> This appears to be not guarded by (require ’engrave-faces-latex nil t).

It is before it’s actually called.

>> -(defun org-latex-src–engrave-code (content lang)
>> -  “Engrave CONTENT to LaTeX in a LANG-mode buffer, and give the result.”
>> +(defun org-latex-src–engrave-code (content lang &optional theme options inline)
>> +  “Engrave CONTENT to LaTeX in a LANG-mode buffer, and give the result.
>> +When THEME is non-nil, it will be used.”
>
> You did not document the remaining two arguments. (this makes me ask a
> question whether you checked compile warnings). Also, consider running
> M-x checkdoc on ox-latex.el.

Now documented.

I have checked compile warnings (there are none), but checkdoc is currently a
bit dodgy on the Emacs commit I’m on.

>> -                (insert content)
>> +                (insert (string-trim-right content “”))
>
> As a theoretical possibility, what will happen if content has something
> like “%”?

Discussed in DMs. It just gets rid of trailing newlines, and it’s fine with
escaping.

All the best,
Timothy