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

allow trailing : in directives for YAML compliance #1312

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

hamelsmu
Copy link
Member

@hamelsmu hamelsmu commented Mar 13, 2023

This is to allow people to work with Quarto a bit easier. People can add a colon after their nbdev directives so it won't break Quarto. This is the most minimal solution I can think of to unblock folks. I tested this as follows:

  • I added a unit test to make sure directives are still extracted correctly, and did some tests on nbdev
  • I tested this on a stand alone quarto project and confirmed that adding a trailing: alleviated Quarto's rendering issue.

This issue keeps cropping on the Quarto side.

This is a workaround for this issue: quarto-dev/quarto-cli#3152

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@hamelsmu
Copy link
Member Author

@cscheid just to confirm, if nbdev directives allowed a trailing : would that allow Quarto to do what it needs to do?

IIUC

foo: is equivalent to foo: null and foo: is valid YAML.

cc: @jjallaire

@cscheid
Copy link

cscheid commented Mar 13, 2023

(With the caveat that I don't have nbdev experience!)

Yes, I believe that would solve the problem on quarto's side. The particular problem isn't when "export" exists by itself on a cell. The problem happens like this:

```{python}
#| export
#| echo: false

print("code")
```

That causes the entire "cell front matter" to become invalid yaml, because export is not a valid key specifier (or whatever the YAML terminology is in this case). (#| export by itself, funnily enough, does work, because a singleton string is a valid YAML object.)

@jjallaire
Copy link

Note that while #| export by itself is valid YAML (the parser doesn't choke) it is interpreted as a string not an object so could cause other problems downstream.

#| export: by itself is valid but the value evaluates to null (which is a bit different than the intended usage which is #| export: true. I guess though if this directive is only used by nbdev then it's entirely fine for it to be interpreted on the quarto side as null.

@hamelsmu
Copy link
Member Author

This is a good point

which is a bit different than the intended usage which is #| export: true

I suppose we might want to allow for that as well in a follow on PR, if folks want to abide by the semantic meaning of YAML.

Jeremy, WDYT?

@hamelsmu
Copy link
Member Author

@jph00 mind taking a look? Thank you

@jph00
Copy link
Member

jph00 commented Mar 23, 2023

Thanks @hamelsmu , I think this is a reasonable workaround for now. Maybe in the future we should consider stuff like #|export foo to be the same as #|export: foo?

@jph00 jph00 merged commit c9a8aa6 into master Mar 23, 2023
@jph00 jph00 deleted the quarto-compatibility branch March 23, 2023 01:24
@cscheid
Copy link

cscheid commented Mar 23, 2023

Fantastic. We'll make sure export: works on our side with the CLI and tooling. Thank you!!

@jph00 jph00 added the enhancement New feature or request label Oct 19, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants