emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
From: Ihor Radchenko <yantar92@posteo.net>
To: Timothy <orgmode@tec.tecosaur.net>
Cc: emacs-orgmode@gnu.org
Subject: Re: [PATCH] Introduce "export features"
Date: Sat, 11 Feb 2023 11:37:04 +0000	[thread overview]
Message-ID: <87lel4bfhb.fsf@localhost> (raw)
In-Reply-To: <875yc95rxp.fsf@tec.tecosaur.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>


  reply	other threads:[~2023-02-11 11:37 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-10 17:20 [PATCH] Introduce "export features" Timothy
2023-02-11 11:37 ` Ihor Radchenko [this message]
2023-02-20 17:41 ` Timothy
2023-02-24 12:51   ` Sébastien Miquel
2023-02-24 12:59     ` Ihor Radchenko
2023-02-24 21:47       ` Sébastien Miquel
2023-02-26 12:19         ` Ihor Radchenko
2023-02-26 13:04           ` Sébastien Miquel
2023-02-27 19:05             ` Ihor Radchenko
2023-02-25  3:15     ` Timothy
2023-02-21 14:22 ` [POLL] Naming of " Timothy
2023-02-22  1:46   ` Dr. Arne Babenhauserheide
2023-02-22  2:40     ` Timothy
2023-02-23 15:55       ` No Wayman
2023-02-23 16:17         ` No Wayman
2023-02-22 12:23   ` Ihor Radchenko
2023-02-23 15:31     ` No Wayman
2023-02-23 16:04     ` Bruce D'Arcus
2023-02-23 19:04       ` Ihor Radchenko
2023-02-23 19:55     ` Sébastien Miquel
2023-02-24 10:27       ` Ihor Radchenko
2023-02-24 12:46         ` Sébastien Miquel
2023-02-24 13:03           ` Ihor Radchenko
2023-02-24 21:38             ` Sébastien Miquel
2023-02-26 12:28               ` Ihor Radchenko
2023-02-26 14:06                 ` Sébastien Miquel
2023-02-27 19:32                   ` Ihor Radchenko
     [not found] <mailman.13.1676134175.1258.emacs-orgmode@gnu.org>
2023-02-11 17:03 ` [PATCH] Introduce " No Wayman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.orgmode.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87lel4bfhb.fsf@localhost \
    --to=yantar92@posteo.net \
    --cc=emacs-orgmode@gnu.org \
    --cc=orgmode@tec.tecosaur.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).