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

feat: execute markdown with jupytext frontmatter #216

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

tlambert03
Copy link
Contributor

@tlambert03 tlambert03 commented Aug 27, 2024

closes #215.

@danielfrg, here is a simple implementation supporting markdown files. As mentioned in #215, the biggest question for me is how best to cleanly add support for markdown files with jupytext frontmatter without causing any issues with "regular" markdown files that don't represent notebooks.

let me know if you have any thoughts

also, if you'd rather not check those two duplicated files for tests, we can probably create them as fixtures using jupytext to convert one of the other files

@danielfrg
Copy link
Owner

Yeah thats the one question I had how to differentiate files, and I like that you need to specify the kernel. that makes it explicit.

Can you add a test with a regular markdown file (no jupytext) too see what happens?
Is there like a hierarchy for which processor would take a file? I am not sure how mkdocs works when multiple plugins try to parse the same file.

@tlambert03
Copy link
Contributor Author

Can you add a test with a regular markdown file (no jupytext) too see what happens?

there is already the index.md file in there ... and when I run rye run mkdocs serve -f material-with-mds.yml the logs don't try to render index.md:

➜ rye run mkdocs serve -f material-with-mds.yml
INFO    -  Building documentation...
INFO    -  Cleaning site directory
INFO    -  The following pages exist in the docs directory, but are not included in the "nav" configuration:
             - backquote_toc_test.ipynb
             - demo-script.py
             - demo.ipynb
             - fail.ipynb
             - ruby.ipynb
             - variational-inference-script.py
             - variational-inference.ipynb
             - extras/README.md
             - extras/styles.py
INFO    -  Converting notebook (execute=False):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/backquote_toc_test.ipynb
INFO    -  Converting notebook (execute=True):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/demo-jupytext.md
INFO    -  Converting notebook (execute=False):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/demo-script.py
INFO    -  Converting notebook (execute=False):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/demo.ipynb
INFO    -  Converting notebook (execute=False):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/fail.ipynb
INFO    -  Converting notebook (execute=False):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/ruby.ipynb
INFO    -  Converting notebook (execute=False):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/variational-inference-script.py
INFO    -  Converting notebook (execute=False):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/variational-inference.ipynb
INFO    -  Converting notebook (execute=True):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/variational-inference.md
INFO    -  Converting notebook (execute=False):
           /Users/talley/dev/forks/mkdocs-jupyter/src/mkdocs_jupyter/tests/mkdocs/docs/extras/styles.py

here's what index.md looks like:

Screenshot 2024-08-27 at 9 28 10 PM

and demo-jupytext.md

Screenshot 2024-08-27 at 9 28 33 PM

anything you'd like me to explicitly test?

@tlambert03
Copy link
Contributor Author

Is there like a hierarchy for which processor would take a file? I am not sure how mkdocs works when multiple plugins try to parse the same file.

that's a good question that I don't have an immediate (knowledgeable) answer for. I can say empirically that it seems to work fine with tests here and the repo i'm actually working on that motivated this PR... but i haven't yet looked yet for the exact line in mkdocs that would clarify why it works

@danielfrg
Copy link
Owner

danielfrg commented Aug 28, 2024

Ok, this looks good to me!

I will hold merging of until I read a bit more about how mkdocs handles that case so we know if this has any unexpected consequences 🙈 . I will try to do that soon.

So we dont render the README.md but maybe the regular md parser tries to parse variational-inference.md?

@tlambert03
Copy link
Contributor Author

I will hold merging of until I read a bit more about how mkdocs handles that case so we know if this has any unexpected consequences 🙈 . I will try to do that soon.

yep, good idea. I may have time today as well. Will update if I learn more

@tlambert03
Copy link
Contributor Author

tlambert03 commented Aug 28, 2024

ok, here's what I've learned:

as per the mdocs Page Events docs:

The pre_page event is called before any actions are taken on the subject page and can be used to alter the Page instance.

(as I'm sure you know :))
so, from that alone, we can pretty much infer that if Plugin.should_include(page.file) returns False, then the page will be returned as is. Side, note, based on this line in mkdocs, you could also have your plugin's on_pre_page method simply return None if not self.should_include(page.file) ... but it should also be fine to return the incoming page as is (which is what this plugin currently does)

with this PR, should_include will return False for all markdown files, unless they specifically have kernel info in their metada. For example, this is what you'd get for the "regular" index.md without any frontmatter:

>>> jupytext.read(page.file.abs_src_path).get('metadata')
{'jupytext': {'main_language': 'javascript', 'notebook_metadata_filter': '-all', 'cell_metadata_filter': '-all', 'text_representation': {'extension': '.md', 'format_name': 'markdown'}}}

there is no kernelspec key at all, so should_include will return False, and we're good.

@hodgestar
Copy link

@tlambert03 @danielfrg I did a short review and this is looking great. Many thanks to both of you. :D

@danielfrg
Copy link
Owner

Ok, this looks good to me then. THank you for the PR and the research!

@danielfrg danielfrg merged commit 7895e56 into danielfrg:main Aug 28, 2024
7 of 8 checks passed
@danielfrg
Copy link
Owner

I will try to get this out later today :)

Copy link

Preview deployment

Name Link
Deploy Preview Url https://c5538bb1.mkdocs-jupyter-danielfrg-com.pages.dev
Latest deploy log https://github.com/danielfrg/mkdocs-jupyter/actions/runs/10603617569
Latest commit 7895e56
Environment production

@tlambert03
Copy link
Contributor Author

Thanks a bunch @danielfrg

@danielfrg
Copy link
Owner

Just released version 0.25.0 with this PR. Thanks!

@hodgestar
Copy link

Thank you!

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.

support for markdown with jupytext frontmatter?
3 participants