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

Improve validation for layout to help distill conversion ? #2013

Open
cderv opened this issue Aug 20, 2022 · 8 comments
Open

Improve validation for layout to help distill conversion ? #2013

cderv opened this issue Aug 20, 2022 · 8 comments
Assignees
Labels
enhancement New feature or request yaml-validation Issues with YAML validation and autocompletion in quarto
Milestone

Comments

@cderv
Copy link
Collaborator

cderv commented Aug 20, 2022

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

  • Content from distill without using YAML syntax
---
title: "A small example"
format: html
page-layout: article
---

```{r setup, layout="l-body-outset"}
mtcars[,1:3] |> head()
```
  • After conversion using knitr::convert_chunk_header()
---
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:
        - C:/Users/chris/AppData/Local/R/win-library/4.2
        - C:/Program Files/R/R-4.2.0/library
      rmarkdown: 2.14.3

[>] Checking Knitr engine render......OK
@cderv cderv added the bug Something isn't working label Aug 20, 2022
@jjallaire jjallaire added this to the v1.2 milestone Aug 21, 2022
@jjallaire jjallaire modified the milestones: v1.2, v1.3 Sep 11, 2022
@cscheid
Copy link
Collaborator

cscheid commented Nov 15, 2022

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

@cderv
Copy link
Collaborator Author

cderv commented Nov 17, 2022

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
Copy link
Collaborator

cscheid commented Nov 17, 2022

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 cscheid added enhancement New feature or request yaml-validation Issues with YAML validation and autocompletion in quarto and removed bug Something isn't working labels Nov 17, 2022
@cscheid
Copy link
Collaborator

cscheid commented Nov 17, 2022

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

@cderv
Copy link
Collaborator Author

cderv commented Nov 17, 2022

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
Copy link
Collaborator

cscheid commented Nov 17, 2022

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
Copy link
Collaborator Author

cderv commented Nov 17, 2022

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
Copy link
Collaborator

cscheid commented Nov 17, 2022

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.

@cscheid cscheid modified the milestones: v1.3, v1.4 Feb 28, 2023
@allenmanning allenmanning modified the milestones: v1.4, Future Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request yaml-validation Issues with YAML validation and autocompletion in quarto
Projects
None yet
Development

No branches or pull requests

4 participants