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

Add test for ipynb as toctree #141

Merged
merged 8 commits into from
Apr 8, 2020
Merged

Add test for ipynb as toctree #141

merged 8 commits into from
Apr 8, 2020

Conversation

chrisjsewell
Copy link
Member

No description provided.

@chrisjsewell chrisjsewell linked an issue Apr 7, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Apr 7, 2020

Codecov Report

Merging #141 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #141   +/-   ##
=======================================
  Coverage   83.85%   83.85%           
=======================================
  Files           9        9           
  Lines         805      805           
=======================================
  Hits          675      675           
  Misses        130      130           
Flag Coverage Δ
#pytests 83.85% <100.00%> (ø)
Impacted Files Coverage Δ
myst_nb/parser.py 91.15% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f0f573...08410a1. Read the comment docs.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Apr 7, 2020

@AakashGfude @mmcky In this PR I added a test for including a toctree in a notebook, and don't find any issues?

You need to modify this test to reproduce the issue you are having.
FYI, if you uncomment the following line, with a suitable path on your machine:

https://github.com/ExecutableBookProject/MyST-NB/blob/2cf7e6467eae1c64daf124d4020a29b1b47412da/tests/test_parser.py#L49

Then run pytest -k test_toctree_in_ipynb,
it will output the full test build there for you to look at (rather than in the default temporary directory)

@chrisjsewell
Copy link
Member Author

The main thing here is that we need a minimal working example of the problem, to identify the issue

@mmcky
Copy link
Member

mmcky commented Apr 7, 2020

@chrisjsewell here is a pretty minimal example

https://github.com/ExecutableBookProject/myst-nb.example-project/tree/minimal

It contains:

  1. conf.py
  2. 1 x document with a single markdown and code cell
  3. 1 x index

Note: this will build. But the pdf contains no content and sphinx throws a warning that ar1_processes cannot be referenced.

@mmcky
Copy link
Member

mmcky commented Apr 7, 2020

So the issue comes about when a notebook contains a code block. As soon as I delete the code cell from ar1_processes document in the above minimal example. The pdf will compile as expected without the sphinx warning that the ar1_processes document is missing.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Apr 7, 2020

From b541b73, this is only an issue for latex (and possibly other non-html) builds.
The initial doctrees are created correctly created, but for some reason other.doctree is not included in the build process in later phases, evidenced by the post-transform CellOutputsToNodes never being called on it.

@mmcky
Copy link
Member

mmcky commented Apr 8, 2020

@chrisjsewell I suspect this will be an issue for:

  1. texinfo
  2. manpage
  3. latex
  4. singlehtml

not that we plan to support manpage and texinfo

It seems to be an issue with inlining the toctree elements with an exception being triggered during the recursive parsing when copying the toctree (#128 (comment)). As that fails I suspect the document doesn't progress to the next phase in sphinx

@mmcky mmcky added the latex label Apr 8, 2020
@mmcky
Copy link
Member

mmcky commented Apr 8, 2020

Ok I think the offending line in docutils is:

obj = self.__class__(rawsource=self.rawsource, **self.attributes)

for error

*** TypeError: __init__() got multiple values for argument 'outputs'

when it tries to unpack **attributes there are two outputs in the method call as **attributes includes {'ids': [], 'classes': [], 'names': [], 'dupnames': [], 'backrefs': [], 'outputs': 0}

and the method has an outputs argument

class CellOutputBundleNode(nodes.container):
    """Represent a MimeBundle in the Sphinx AST, to be transformed later."""

    def __init__(self, outputs, rawsource="", *children, **attributes):
        self.outputs = outputs
        attributes["outputs"] = len(outputs)
        super().__init__("", **attributes)

@chrisjsewell
Copy link
Member Author

Thanks @mmcky, this should be fixed in ebd9363

@@ -230,7 +230,7 @@ def render_nb_code_cell(self, token: Token):
# ==================
# Cell output
# ==================
if "remove_output" not in tags:
if "remove_output" not in tags and cell["outputs"]:
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I now skip the creation of the CellOutputNode if the code cell has no outputs. @choldgraf any reason you can think of why this should not be done?

Copy link
Member

Choose a reason for hiding this comment

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

Nah that sounds like a good idea to me

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@mmcky
Copy link
Member

mmcky commented Apr 8, 2020

perfect thanks @chrisjsewell just got the pdf building by changing the count variable name.

@chrisjsewell chrisjsewell marked this pull request as ready for review April 8, 2020 02:23
@chrisjsewell chrisjsewell merged commit 61c2d71 into master Apr 8, 2020
@chrisjsewell chrisjsewell deleted the ipynb-toctree branch April 8, 2020 02:47
phaustin pushed a commit to phaustin/MyST-NB that referenced this pull request Apr 10, 2020
This commit makes the sphinx based test fixture more general, and fixes a bug in `CellOutputBundleNode`, whereby there was an attribute clash with `outputs`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using an ipynb as the root (index) file
3 participants