Add (and fix) round_trip_tests_for_MyST_markdown #1345
Conversation
|
Thank you for making this pull request. Did you know? You can try it on Binder: Also, the version of Jupytext developed in this PR can be installed with (this requires |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1345 +/- ##
==========================================
+ Coverage 96.84% 97.03% +0.19%
==========================================
Files 32 32
Lines 4906 4924 +18
==========================================
+ Hits 4751 4778 +27
+ Misses 155 146 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if ( | ||
| is_myst_available() | ||
| and ext in myst_extensions() | ||
| and matches_mystnb(text, ext, requires_meta=False) |
There was a problem hiding this comment.
The previous setting requires_meta=True was the cause of the unstable round trips observed at #789 (comment)
src/jupytext/jupytext.py
Outdated
| if default_lexer is None: | ||
| language = metadata.get("kernelspec", {}).get( | ||
| "language" | ||
| ) or metadata.get("jupytext", {}).get("main_language") |
There was a problem hiding this comment.
Notebooks constructed from minimalist MyST notebooks don't have any explicit metadata. When reading the MyST notebooks we will now parse the lexer and identify the notebook language from them.
There was a problem hiding this comment.
Changed to jupytext.default_lexer which unlike the above will work for all languages
src/jupytext/myst.py
Outdated
| @@ -348,6 +370,16 @@ def _flush_markdown(start_line, token, md_metadata): | |||
|
|
|||
| if add_source_map: | |||
| notebook.metadata["source_map"] = source_map | |||
|
|
|||
| if "kernelspec" not in notebook.metadata: | |||
| for language, count in languages.most_common(1): | |||
There was a problem hiding this comment.
@choldgraf @chrisjsewell we don't expect multiple languages or lexers in the same notebook right? Would you prefer to raise if we observe multiple languages to avoid the risk of using a different language than the one intended?
There was a problem hiding this comment.
Changed to default_lexer equal to first non null lexer. There will be a warning if the lexers are not unique.
There was a problem hiding this comment.
In the near term, I don't think anyone is making use of the language feature for anything beyond syntax highlighting. And, there is current precedent for notebook frontends like JupyterLab to pass that information down to the kernel. There are multi-language kernels like SOS, but they patch the frontend to inject the context about which language is being executed, so they're fairly bespoke.
Given this situation, I'd be inclined to state that MyST notebooks are for-now single language, and raise an error. We can unwind this in future if we need. As far as the MyST-MD engine goes, I think we'd like for language to be optional for the cell, meaning that it should be possible to define it exclusively in the metadata / kernelspec.
I'd also be inclined to say that it is possible to interrogate the on-disk kernelspec for the language, but that introduces an "on-my-system" dependency that I don't think jupytext currently wants to do.
mwouts
force-pushed
the
round_trip_tests_for_myst_markdown
branch
from
acfe47d to
67c7181
Compare
|
@choldgraf @chrisjsewell can you confirm you're fine with this change? In a nutshell I am adding a |
This PR will eventually close #1317 and #759