quarto-dev / quarto-cli

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

Improve validation for layout to help distill conversion ? #2013

Open cderv opened 2 years ago

cderv commented 2 years ago

Bug description

distill has also a layout option, and for now if the option is not correctly converted to Quarto supported one then there is an useful error

mtcars[,1:3] |> head()

* After conversion using `knitr::convert_chunk_header()`
````markdown
---
title: "A small example"
format: html
page-layout: article
---

```{r}
#| layout: l-body-outset
mtcars[,1:3] |> head()

in both case, we'll have this issue 

Error running filter C:/Users/chris/scoop/apps/quarto-prerelease/current/share/filters/layout/layout.lua: ...prerelease\current\bin..\share\pandoc\datadir_json.lua:167: bad 'for' initial value (number expected, got nil) stack traceback: ...prerelease\current\bin..\share\pandoc\datadir_json.lua:381: in function '_json.decode' ...uarto-prerelease/current/share/filters/layout/layout.lua:3641: in function 'parseLayoutWidths' ...uarto-prerelease/current/share/filters/layout/layout.lua:4274: in function 'layoutCells' ...uarto-prerelease/current/share/filters/layout/layout.lua:4098: in function <...uarto-prerelease/current/share/filters/layout/layout.lua:4091> ERROR: unexpected character 'l' at line 1 col 1


Not so useful. 
And in the second case, YAML validation does not help to detect that. No error detected for the `layout` field in the chunk for me. 

Can we do better in the error or at least in the validation ?  

### `quarto check` Output

$ quarto check

[>] Checking Quarto installation......OK Version: 1.1.80 Path: C:\Users\chris\scoop\apps\quarto-prerelease\current\bin\ CodePage: unknown

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

[>] Checking Python 3 installation....OK Version: 3.9.13 Path: C:/Users/chris/scoop/apps/pyenv/current/pyenv-win/versions/3.9.13/python3.exe Jupyter: 4.11.1 Kernels: bash, julia-1.7, python3

[>] Checking Jupyter engine render....OK

() Checking R installation...........++ Activating rlang global_entrace

++ Setting QUARTO_PYTHON

[>] Checking R installation...........OK Version: 4.2.0 Path: C:/PROGRA~1/R/R-42~1.0 LibPaths:

[>] Checking Knitr engine render......OK

cscheid commented 1 year ago

@cderv You'll need to explain this one to me, specifically the interaction with distill

cderv commented 1 year ago

Sure.

As first step, here is distill doc site : https://rstudio.github.io/distill/

We have a specific layout chunk option (https://rstudio.github.io/distill/figures.html) that can contains some value not supported by Quarto. I was thinking that our validation can warn about those known possible values that one would end up with after converting.

I could also create an R function in distill to help convert the chunk option so that we correctly use the new layout option from Quarto which require a new value.

cscheid commented 1 year ago

I see. This requires a new validation feature ("warning" schemas instead of error schemas), so it'll take a while. We're probably not going to have it in 1.3

cscheid commented 1 year ago

We should probably track the bug itself (which is the option not being respected) in a different issue.

cderv commented 1 year ago

I think we could go with an error on this. This is not option not being respected. Let me clarify what i mean

Same option name for different purpose.

layout should be in Quarto

2d-array of widths where the first dimension specifies columns and the second rows.

But currently no error from YAML side when something like a string is passed to the field

#| layout: l-body-outset

Should be an error right ?

For the correct translation from layout="l-body-outset" from distill to equivalent quarto feature (column: body-outset), I think in fact that is should be in distill or knitr

Is that clearer maybe ?

cscheid commented 1 year ago

Yes, it's clearer. Unfortunately, that is hard to do because cell schemas are currently tied to the engine and not the format (it should be possible to do both, but that is hard to fix right now.)

cderv commented 1 year ago

cell schemas are currently tied to the engine and not the format

Not sure to understand correctly what you mean by that but you are the YAML validation expert !

Glad I manage to explain it correctly. I'll try to improve things from distill side too, maybe that will be easier as this will happen mainly for user converting from distill format.

cscheid commented 1 year ago

Not sure to understand correctly what you mean by that but you are the YAML validation expert !

The execution engine (jupyter vs knitr) controls the schemas that are allowed on an execution cell. For code cells, we have jupyter.yml and knitr.yml. For document and project metadata, we have html.yml and distill.yml (sort of). But it's not possible, right now, for html.yml to have a say about the yaml values in a code cell.