quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.95k stars 326 forks source link

Typst: 1.6 introduces Pandoc interpolated variable to `typst-template.typ` rather than `typst-show.typ` #10212

Closed christopherkenny closed 4 months ago

christopherkenny commented 4 months ago

Bug description

A change in this commit introduces one Pandoc interpolated variable into typst-template.typ. While this is technically fine, as it really is a Pandoc template and valid code, this should probably be done in typst-show.typ with all other Pandoc variables. As a bonus, this is makes Typst LSPs angry, because it makes the Pandoc-ness more apparent, and makes editing Typst templates a bit messier.

Happy to PR in the minor changes necessary discussed below, if that's useful. I would have just opened a PR, but I didn't know if this was an intentional choice.

Finally, apologies if you view this more as a Feature Request as it technically isn't a bug, just a slightly off behavior. Happy to move to discussions if that's preferable.

Steps to reproduce

Create a Typst format with

quarto create extension format:typst

And give it a name.

Line 64 of typst-template.typ now looks like below, which introduces a pandoc variable into the file.

    #text(weight: "semibold")[$labels.abstract$] #h(1em) #abstract

For testing completeness, this is just a document with an abstract, but any will do:

---
title: Untitled
abstract: |
  Typst is a pretty cool way to produce PDFs. Currently the `typst-template.typ` 
  file has a single pandoc interpolted variable which could be a variable
  in `typst-show.typ` to let `typst-template.typ` play nice in Typst LSPs.
format:
  typtest-typst: default
---

## Introduction

*TODO* Create an example file that demonstrates the formatting and features of your format.

## More Information

You can learn more about creating custom Typst templates here: 

<https://quarto.org/docs/prerelease/1.4/typst.html#custom-formats>

Expected behavior

All other partials introduce variables in typst-show.typ. I would expect to see:

In the article definition in typst-template.typ

  abstract: none,
  abstract-title: none,
  cols: 1,

Within the article body:

    #text(weight: "semibold")[#abstract-title] #h(1em) #abstract

In typst-show.typ:

$if(abstract)$
  abstract: [$abstract$],
  abstract-title: "$labels.abstract$",
$endif$

Actual behavior

No variables in Typst are created, the Pandoc interpolated variable is dropped into the article definition like so:

    #text(weight: "semibold")[$labels.abstract$] #h(1em) #abstract

Your environment

Quarto check output

Quarto 1.6.0
[✓] Checking versions of quarto binary dependencies...
      Pandoc version 3.2.0: OK
      Dart Sass version 1.70.0: OK
      Deno version 1.41.0: OK
      Typst version 0.11.0: OK
[✓] Checking versions of quarto dependencies......OK
[✓] Checking Quarto installation......OK
      Version: 1.6.0
      Path: /Applications/quarto/bin

[✓] Checking tools....................OK
      TinyTeX: (external install)
      Chromium: (not installed)

[✓] Checking LaTeX....................OK
      Using: TinyTex
      Path: /Users/chris/Library/TinyTeX/bin/universal-darwin
      Version: 2024

[✓] Checking basic markdown render....OK

[✓] Checking Python 3 installation....OK
      Version: 3.12.3
      Path: /usr/local/opt/python@3.12/bin/python3.12
      Jupyter: (None)

      Jupyter is not available in this Python installation.
      Install with python3 -m pip install jupyter

[✓] Checking R installation...........OK
      Version: 4.4.1
      Path: /Library/Frameworks/R.framework/Resources
      LibPaths:
        - /Library/Frameworks/R.framework/Versions/4.4-x86_64/Resources/library
      knitr: 1.46
      rmarkdown: 2.27

[✓] Checking Knitr engine render......OK
cscheid commented 4 months ago

Thanks for opening this.

I don't want to promise that we'll take the PR sight unseen, because I'd like to know the full consequences first. We currently don't prioritize making the Typst LSP happy at all in templates, but that's roughly because we haven't considered it.

I think we still want our templates to be well-structured for our goals first, and I don't know how far we'd go in the other direction. But if there are changes that make your use case better and don't really affect the overall structure of our templates (or maybe even improves it!), I think we would be happy making the change.

gordonwoodhull commented 4 months ago

This change makes sense to me. I wasn't aware of the convention of introducing variables in typst-show.typ when I implemented i18n here.

christopherkenny commented 4 months ago

Thank you for the replies.

Focusing on what's best for Quarto makes a lot of sense to me. (If you wanted to make the Typst LSP + everyone else happy, it'd be an unwinnable battle...)

As the template itself itself is really building a big function call, I think that, generally, passing Pandoc variables to Typst (in typst-show.typ) is the most ergonomic. It makes the translation of variables very explicit and keeps them together for the two largest partials. I would think the major drawback of having typst-show.typ and typst-template.typ separate in the first place is that you have to edit them in concert or the compilation will fail, whereas the other partials (e.g. bibliography.typ) are more independent. In that sense, I see an argument for why you may not want to separate Pandoc and Typst variables in this way.

I'll open a PR so that you can see it in context and be able to run any checks or other things. There are no hurt feelings if you decide that it's not best for the default template.

On the default note, I plan to open a discussion over the holiday with some specific thoughts on improving the default template. For example, things like linkcolor for PDFs (when made with LaTeX) are extremely simple to add support for in Typst, but are not currently included.

gordonwoodhull commented 4 months ago

Thanks @christopherkenny! Looking forward to your contributions!

gordonwoodhull commented 4 months ago

Fixed by #10220