Re: [PATCH] Introduce "export features"

[Ihor Radchenko <yantar92@posteo.net>]

Timothy <orgmode@tec.tecosaur.net> writes:

> “export features” allow for the specification of qualities of the org buffer
> being exported that imply certain “features”, and how those features may be
> implemented in a particular export.
>
> This is done by augmenting the backend struct with two new fields:
> `feature-conditions' and `feature-implementations'.
>
> The feature conditions are resolved during the annotation of `info', in the Org
> buffer after `#+include' expansion and the removal of comments.
>
> The feature implementations are expanded by the backend itself, in the case of
> `ox-latex' this currently means during preamble construction.

I am in favour of this.

Some comments on the code are below.

> +  `(("^[ \t]*#\\+print_bibliography:" . bibliography)

This will also match bibliography statements, which are not keywords.
For example, inside quote blocks.

> +  :group 'org-export-general
> +  :type '(alist :key-type
> +                (choice (regexp :tag "Feature test regexp")
> +                        (variable :tag "Feature variable")
> +                        (function :tag "Feature test function"))
> +                :value-type
> +                (choice (symbol :tag "Feature symbol")
> +                        (repeat symbol :tag "Feature symbols"))))

:package-version is missing in this defcustom. It should be added.

> +(defun org-export-detect-features (info)
> +  "Detect features from `org-export-conditional-features' in INFO."
> +  (let (case-fold-search)

This unconditionally sets case-sensitive search. Thus, for example,
#+PRINT_BIBLIOGRAPHY (as opposed to #+print_bibliography) will not be
recognized as 'bibliography feature.

> +    (delete-dups
> +     (mapcan
> +      (lambda (construct-feature)

This lambda could be a private function instead. A function would be
byte-compiled.

>  (defcustom org-latex-default-packages-alist
>    '(("AUTO" "inputenc"  t ("pdflatex"))
>      ("T1"   "fontenc"   t ("pdflatex"))
> -    (""     "graphicx"  t)

You need to update :package-version upon changing the defcustom value.

> +;;;; Generated preamble
> +
> +(defcustom org-latex-conditional-features

defcustom :keywords are missing here. Please, add.

> +    (,(lambda (info)
> +        (org-element-map (plist-get info :parse-tree)
> +            'link
> +          (lambda (link)
> +            (and (member (org-element-property :type link)
> +                         '("http" "https" "ftp" "file"))
> +                 (file-name-extension (org-element-property :path link))
> +                 (equal (downcase (file-name-extension
> +                                   (org-element-property :path link)))
> +                        "svg")))
> +          info t))
> +     . svg)

This is a duplicate of an entry in `org-export-conditional-features'. Is
it intentional?

> +(defcustom org-latex-feature-implementations

:package-version is missing.

> +  "Alist describing how export features should be supported in the preamble.
> +
> +Implementation alist has the feature symbol as the car, with the
> +cdr forming a plist with the following keys:
> +- :snippet, which is either,
> +  - A string, which should be included in the preamble verbatim.
> +  - A variable, the value of which should be included in the preamble.
> +  - A function, which is called with two arguments — the export info,
> +    and the list of feature flags. The returned value is included in
> +    the preamble.
> +- :requires, a feature or list of features which are needed.
> +- :when, a feature or list of features which imply this feature.
> +- :prevents, a feature or list of features that should be masked.
> +- :order, for when inclusion order matters. Feature implementations
> +  with a lower order appear first.  The default is 0."
> +  :group 'org-export-general

What is this group?

> +  :type '(plist :key-type
> +          (choice (const :snippet)
> +                  (const :requires)
> +                  (const :when)
> +                  (const :prevents)
> +                  (const :order)
> +                  (const :trigger))
> +          :value-type
> +          (choice (string :tag "Verbatim content")
> +                  (variable :tag "Content variable")
> +                  (function :tag "Generating function"))))

The docstring and :type are rather generic wrt backend. Can they be
abstracted away?

> -(defun org-latex-guess-inputenc (header)
> +(defun org-latex-guess-inputenc (info)

It is a breaking change. Can we make something to not break existing
third-party code?

> -(defun org-latex-guess-babel-language (header info)
> +(defun org-latex-guess-babel-language (info)

Again, backwards-incompatible.

> -(defun org-latex-guess-polyglossia-language (header info)
> +(defun org-latex-guess-polyglossia-language (info)

backwards-incompatible

> +    (concat
> +     ;; Time-stamp.
> +     (and (plist-get info :time-stamp-file)
> +          (format-time-string "%% Created %Y-%m-%d %a %H:%M\n"))

May it also be a feature?

> +     ;; LaTeX compiler.
> +     (org-latex--insert-compiler info)

And this.

> +(defun org-export-update-features (backend &rest feature-property-value-lists)
> +  "For BACKEND's export spec, set all FEATURE-PROPERTY-VALUE-LISTS.

what about

(defun org-export-update-features (backend &rest feature-settings)
  "Update FEATURE-SETTINGS for BACKEND.

> +Specifically, for each (FEATURE . PROPERTY-VALUE-LIST) entry of
> +FEATURE-PROPERTY-VALUE-LISTS, each :PROPERTY VALUE pair of
> +PROPERTY-VALUE-PAIRS is set to VALUE within the backend's feature
> +implementation plist.  The sole exception to this is the
> +:condition property, the value of which is set in the backend's
> +feature condition plist instead.

This patch is not what how it looks at the end, right? One of the
following patches completely amends the docstring.

Could you please restructure the patches in a way that is easier to
review? Without significant changes in implementations.

I am not sure if all the above comments will apply to the final
patchset.

Also, you will need to add ORG-NEWS entry and document the new feature
system in "13.17 Advanced Export Configuration" and "A.4 Adding Export
Back-ends" sections of the manual.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
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>