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

fixing jupytext markdown inconsistencies #288

Merged
merged 2 commits into from Sep 2, 2019

Conversation

choldgraf
Copy link
Member

This changes the logic for reading in text files in order to be more consistent about how markdown files are handled.

The current situation:

Any content file will be read in with jupytext. This is awesome, with the exception that some people's code blocks may be incorrectly parsed as code cells in the markdown files. In the short term, it'd be tricky to avoid that behavior from within juyptext

This PR's approach:

If a .markdown or .md file is found, read it in to memory. First see if it has any YAML header.

  • If so, check if yaml.get('jupyter').get('jupytext') returns None.
    • If so, or if there was no YAML, then assume it is a "regular" markdown file and just dump it into a single NotebookNode markdown cell.
    • If not (AKA, there's something in yaml.jupyter.jupytext) then use jupytext.reads to load in the notebook.

The main assumption this is making is that jupytext files will always have a YAML header with something in the jupyter.jupytext field, while non-jupytext markdown files will not. That seems like a reasonable assumption to make.

@mwouts what do you think about this implementation? It's a bit hacky, but at least in the short-term it solves the markdown parsing differences problem. Perhaps longer-term we can figure out a way to solve this more gracefully!

else:
# Otherwise, create an empty notebook and add all of the file contents as a markdown file
ntbk = nbf.v4.new_notebook()
ntbk['cells'].append(nbf.v4.new_markdown_cell(source=''.join(content)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Chris, yes sure, at least this way we're going to fix the rendering of the Jupyter Book book itself. I think this is good enough for a quick fix, and as you suggest we will follow-up on this.

language: python
name: python3
---

# Creating book content
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe changing this file is not necessary.

And the changes I see below (with Markdown comments surrounding the python code cells) raise an interesting question: should Python code blocks in the Markdown file always appear as executable code cells? As you noticed, these Markdown comments allow to preserve Jupyter's Markdown cells whatever there content is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I needed to change the file so that I could use the right sidebar popout, which Jupyter Book can only use with notebooks instead of markdown files, this is why I added the jupytext header. I'll put my thoughts about choosing markdown syntax in the issue so that we don't lose the conversation when this is merged!

@choldgraf
Copy link
Member Author

OK, merging this one since the current master branch is wonky due to markdown processing inconsistencies! It's a bit hacky but it'll get the job done :-) and perhaps @mwouts and I can figure out a more graceful solution to some of these syntax questions! (thanks for you help figuring this out @mwouts!!)

@choldgraf choldgraf merged commit 33e7cdf into executablebooks:master Sep 2, 2019
@choldgraf choldgraf deleted the jupytext_fix branch September 2, 2019 15:15
@choldgraf choldgraf added the bug Something isn't working label Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants