Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement defaults file inheritance #6924

Merged
merged 1 commit into from
Jan 5, 2021
Merged

Conversation

davidmrt98
Copy link
Contributor

This pull request adds an inheritance system for defaults files as requested in #6827.

Motivation

Currently, it is not possible to share a set of options between defaults files in a flexible way. The inheritance system implemented in this branch solves that problem by allowing a defaults file to specify other defaults files to inherit from.

Usage examples

Simple example

See #6827

Multiple defaults files

Assume defsA.yaml looks like this:

defaults:
  - defsB
  - defsC
from: html

Then calling pandoc -d defsA has the same result as calling pandoc -d defsB -d defsC -d defsA. Note that first defsB.yaml is "applied", followed by defsC.yaml and finally defsA.yaml.

Circular references

Assume:

  • defsA.yaml:
defaults:
  - defsB
  • defsB.yaml:
defaults:
  - defsA

In this scenario defsA.yaml inherits from defsB.yaml and vice versa. As there is no sensible way of resolving this circularity, executing pandoc -d defsA yields the following error message: "Error: Circular defaults file reference in 'defsB.yaml'.

User data directories

Assume:

  • defsA.yaml:
defaults:
  - defsB
data-dir: PATH_TO_DATA_DIR_B
template: test
  • defsB.yaml:
defaults:
  - defsC
data-dir: PATH_TO_DATA_DIR_C

When calling pandoc -d defsA, pandoc searches for defsB.yaml and test.html in PATH_TO_DATA_DIR_B/defaults and for defsC.yaml in PATH_TO_DATA_DIR_C/defaults. In other words: specifying a user data directory in defsB.yaml does not affect defsA.yaml.

Implementation

The feature is implemented by adjusting/extending the FromYAML (Opt -> Opt) instance in Pandoc/App/Opt.hs. I have tried to change as little as possible, but since parsing a YAML file now potentially requires file-IO and keeping track of a graph-like data structure in order to detect circular inheritance, I had to introduce MonadIO and StateT to some type signatures.

closes #6827

@jgm
Copy link
Owner

jgm commented Dec 5, 2020

Thanks for this! On the whole it looks good. I've got a few comments on a first glance through.

  1. You'll need to add the .yaml files in command tests to the extra-source-files stanza of the cabal file. There is currently no catch-all for these. Alternatively we could add a wildcard test/command/*.yaml rule.

  2. Removing the FromYAML instance for Opt -> Opt would require a bump in major API version. At the least, the commit message should indicate this so we don't lose track. Another option, more backwards compatible, would be to preserve this instance. Would it hurt to keep it?

  3. The use of PandocSomeError is a bit of a code wart, though we do use it elsewhere in the code base. If (but only if) we are going to have an API bump (see Cannot omit libraries from install #2), then I'd suggest adding a new constructor to PandocError that specifically targets circular dependencies in default files.

  4. In parseYAML you convert the map to a list, I assume, so that inexact matching can be done (because we don't know the exact Scalar representing the key, only its Text component). Another alternative would be to use M.mapKeys to change the keys to Text, and then use M.lookup. I doubt it really makes a measurable difference in performance which way we go, esp. considering how small these files are, so this is a minor issue and the current code can be left as is if you think it's best. I just wanted to raise the possibility.

  5. Is it possible to simplify parseOptions? At least we could have return $ return . f, but is it not possible to use foldM instead of combinig mapM and foldr?

  6. In your PR explanatory text you helpfully note the order in which defaults are applied, but this should go in the manual, perhaps just in the commented section with defaults:.

@davidmrt98 davidmrt98 force-pushed the defaults-inheritance branch from cc7f472 to b600e80 Compare December 7, 2020 15:54
@davidmrt98
Copy link
Contributor Author

Thanks for the quick response! I have addressed your points, I hope it looks better now.

regarding 2.) I simply did not consider that removing the instance would affect backwards compatibility. Keeping it should not be a problem!

regarding 3.) I was thinking that using PandocSomeError is not the cleanest solution. Adding a constructor to PandocError would be nice, but that is probably off the table now (since we keep the Opt -> Opt instance).

@jgm
Copy link
Owner

jgm commented Dec 7, 2020

Thanks, this is looking good. I had one more thought. Why not allow defaults to be a scalar value, too? So either

defaults: foo

or

defaults:
- foo

is possible. See how we currently handle css in defaults files, for example.

@davidmrt98 davidmrt98 force-pushed the defaults-inheritance branch from b600e80 to b9fdd90 Compare December 8, 2020 20:07
@davidmrt98
Copy link
Contributor Author

Done! I have also added a new command test for this.

Allow defaults files to inherit options from other defaults files by
specifying them with the following syntax:
'defaults: [list of defaults files]'.
@jgm jgm merged commit 385b6a3 into jgm:master Jan 5, 2021
@jgm
Copy link
Owner

jgm commented Jan 5, 2021

Great, thanks for your contribution to pandoc!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow defaults files to inherit options from other defaults files
2 participants