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

✨ NEW: Introducing jupyterbook-latex and added handling of tableofcontents for latex #1167

Merged
merged 38 commits into from
Mar 12, 2021

Conversation

AakashGfude
Copy link
Contributor

@AakashGfude AakashGfude commented Jan 7, 2021

This PR brings a range of improvements to PDF builds via LaTeX by incorporating the jupyterbook-latex extension. This initial release is focused on bringing harmony to html and latex outputs (through support for part/chapter structures in the _toc.yml), support for the jupyter-book directive tableofcontents, and support for commonly used tags on code-cell directives such as hide-cell.

This initial release targets whole of project pdf files only.

  1. adds the jupyterbook-latex extension which provides improvements for pdf and latex generation. Activation of this package is configurable with the use_jupyterbook_latex key (turned on by default). Further information for jupyterbook-latex can be read here: https://github.com/executablebooks/jupyterbook-latex/blob/master/docs/intro.md. This package adds support for document structures that are recommended for jupyter-book
  2. adds support for the tableofcontents directive for LaTeX by creating a list of reference nodes such as.

Screen Shot 2021-02-02 at 10 56 54 am

We have documented some design decisions and future work in this discussion thread.

TODO:

  • only include entries that are children of the document in which directive is used
  • Add tests

fixes #847
fixes #810
fixes #1192

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #1167 (7161388) into master (5de7ca6) will increase coverage by 0.72%.
The diff coverage is 95.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1167      +/-   ##
==========================================
+ Coverage   92.78%   93.50%   +0.72%     
==========================================
  Files           8        8              
  Lines         887      970      +83     
==========================================
+ Hits          823      907      +84     
+ Misses         64       63       -1     
Flag Coverage Δ
pytests 93.50% <95.95%> (+0.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jupyter_book/directive/toc.py 96.22% <95.74%> (+6.94%) ⬆️
jupyter_book/__init__.py 100.00% <100.00%> (ø)
jupyter_book/config.py 94.91% <100.00%> (+0.05%) ⬆️
jupyter_book/toc.py 94.41% <0.00%> (+0.55%) ⬆️
jupyter_book/sphinx.py 86.25% <0.00%> (+1.25%) ⬆️

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 5de7ca6...7161388. Read the comment docs.

@AakashGfude AakashGfude force-pushed the latex-tocd branch 2 times, most recently from cafa391 to 6681b7e Compare February 1, 2021 01:29
@AakashGfude AakashGfude marked this pull request as ready for review February 1, 2021 01:39
@AakashGfude AakashGfude changed the title 👌 IMPROVE: Added handling of tableofcontents for latex 🚀 NEW: Introducing jupyterbook-latex and added handling of tableofcontents for latex Feb 1, 2021
@AakashGfude
Copy link
Contributor Author

Have also introduced jupyterbook_latex here as well, the reason for which is outlined in #1125 .

The styling of tableofcontents directive follows the default styling of sphinx for now. (the title 'Table of Contents' is part of markdown here not the directive)
Screen Shot 2021-02-01 at 1 23 36 pm

cc : @mmcky @choldgraf @chrisjsewell

@mmcky
Copy link
Contributor

mmcky commented Feb 1, 2021

@AakashGfude this is looking good. May I suggest you edit the top (main) comment field with a full description including the inclusion of jupyterbook-latex? That will make it easier for putting together releases as this is a big improvement for pdf outputs. Will this also include sphinx-multitoc-numbering and change default numbering behaviour? Let's add a section for any proposed changes to defaults etc.

This will supersede #1125 right?

@AakashGfude AakashGfude changed the title 🚀 NEW: Introducing jupyterbook-latex and added handling of tableofcontents for latex ✨ NEW: Introducing jupyterbook-latex and added handling of tableofcontents for latex Feb 1, 2021
@AakashGfude
Copy link
Contributor Author

@AakashGfude this is looking good. May I suggest you edit the top (main) comment field with a full description including the inclusion of jupyterbook-latex? That will make it easier for putting together releases as this is a big improvement for pdf outputs. Will this also include sphinx-multitoc-numbering and change default numbering behaviour? Let's add a section for any proposed changes to defaults etc.

This will supersede #1125 right?

Thanks @mmcky , have updated the top comment. No, I have not introduced spinx-multitoc-numbering with this. Will create a new PR for that.
Yes, it will supersede #1125 .

I have included the doc page, where all the features are outlined. Hopefully, that can outline the proposed changes?
We can also edit the docs if some things are not clear.

Copy link
Contributor

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

Looking good cheers, just a request for documenting/typing the code a bit more, and some more "lower level" unit tests

return filtered_toc
return

def _has_toc_yaml(self, subnode, tocdict, depth):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method, together with _handle_toc_header, should be a standalone function. Then you can you add some unit tests, e.g. constructing example globaltoc dicts and testing the output subnode.pformat(). Because at the moment there is only a few integration tests where it is difficult to test a large range of possibilities and to understand what the functions do.

Also can you try "typing" your methods and providing docstring explanations of the inputs, e.g. something like

from typing import Dict

from docutils import nodes

def _has_toc_yaml(self, subnode: nodes.Element, tocdict: Dict[str, nodes.Element], depth: int) -> None:
   """constructs toc nodes from globaltoc dict

	:param subnode: this is....
    :param tocdict: this is....
    :param depth: this is....
   """

You should be trying to type all your code and using mypy, for example markdown-it-py and myst-parser are now fully typed (executablebooks/MyST-Parser@07ac859) and this will eventually be the same for myst-nb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. Thanks, @chrisjsewell . On it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @chrisjsewell, have typed the functions and added tests for individual ones. let me know if it is looking better.

@chrisjsewell
Copy link
Contributor

Also in https://github.com/executablebooks/jupyterbook-latex/blob/master/docs/intro.md it says "jupyterbook-latex is in an active development stage and may change rapidly.", and it says about "git cloning" to install.

a) With these changes, is this package more stable now, and so we don't have to essentially warn people away from using anymore
b) Is there a way we can have an example build that be can be linked to from the documentation, prefereably of the actual jupyter-book documentation, so that people can see for themselves what the output looks like
c) given jupyter-latex is now part of the required installs, we can probably remove the installation instructions
d) also given it is part of the required installs, we need it to have a conda-forge package. Is this the case?

@mmcky
Copy link
Contributor

mmcky commented Feb 11, 2021

@AakashGfude is TexSoup a dependency that needs to be added?

@AakashGfude
Copy link
Contributor Author

@AakashGfude is TexSoup a dependency that needs to be added?

@mmcky have added the dependency here https://github.com/executablebooks/jupyter-book/pull/1167/files#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7R20 , only for the testing case.

jupyter_book/directive/toc.py Show resolved Hide resolved
@mmcky
Copy link
Contributor

mmcky commented Feb 11, 2021

@AakashGfude is TexSoup a dependency that needs to be added?

@mmcky have added the dependency here https://github.com/executablebooks/jupyter-book/pull/1167/files#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7R20 , only for the testing case.

Hmm ... I just had a failure when using tox due to TexSoup missing.

@chrisjsewell
Copy link
Contributor

Hmm ... I just had a failure when using tox due to TexSoup missing.

did you run tox -r to update the environment?

@mmcky
Copy link
Contributor

mmcky commented Feb 11, 2021

@AakashGfude doing some testing on lecture-python.myst I am finding:

  1. unicode errors issued by latex (I know you're looking into this - as per Slack discussion)
  2. execution via jupyter-cache seems to run fresh each time.

I'll continue to dig a little into these issues

@AakashGfude
Copy link
Contributor Author

AakashGfude commented Feb 11, 2021

@AakashGfude doing some testing on lecture-python.myst I am finding:

  1. unicode errors issued by latex (I know you're looking into this - as per Slack discussion)

oopsie. sorry had not setup jupyterbook-latex as an extension in __init__ of this PR. have done it now. 61f4cb9
So, should use the extension now by default.

  1. execution via jupyter-cache seems to run fresh each time.

Since your build was not using jupyterbook-latex earlier, I suspect this issue might not be related to this PR then.🤪

@mmcky
Copy link
Contributor

mmcky commented Feb 11, 2021

thanks @AakashGfude building nicely now after git pull -- I will review the pdf once it is built. Also the execution is no longer running each time 🥳

@AakashGfude
Copy link
Contributor Author

Also in https://github.com/executablebooks/jupyterbook-latex/blob/master/docs/intro.md it says "jupyterbook-latex is in an active development stage and may change rapidly.", and it says about "git cloning" to install.
a) With these changes, is this package more stable now, and so we don't have to essentially warn people away from using anymore

I belive with our testing in different projects its stable enough. So, I reckon we can remove this.

b) Is there a way we can have an example build that be can be linked to from the documentation, preferably of the actual jupyter-book documentation, so that people can see for themselves what the output looks like.

Should we add a button to the theme, to download the whole repo as pdf using this extension as well?

c) given jupyter-latex is now part of the required installs, we can probably remove the installation instructions.

have done these changes in this PR https://github.com/executablebooks/jupyterbook-latex/pull/37/files#diff-22760d417a52c66f14bee182587d458d0737a616dd14cb09748b4c725fc5809fR12 .

@mmcky
Copy link
Contributor

mmcky commented Feb 12, 2021

Should we add a button to the theme, to download the whole repo as pdf using this extension as well?

@AakashGfude perhaps there should be an option that users can switch on for sphinx-book-theme to be powered either by javascript or by latex for pdf support. However as jupyterbook-latex currently doesn't support individual pdf files yet -- we should wait on this feature so it is a seamless switch (should someone want to be latex powered). Also given the additional tex engine requirements I suspect the default should be javascript based. So I think it will be OK for now for users to fetch a latex based pdf using jb build ./ --builder=pdflatex and going to _build/latex. What do you think?

But an example link would be great 👍. We could build the pdf in the github actions and copy it as an asset that we can link to in the docs as sample output.

@mmcky
Copy link
Contributor

mmcky commented Feb 12, 2021

@AakashGfude is there a way to switch this feature off just in case a user has configured something that they are using with the existing setup? I think this should be switched on by default to support files and part/chapter structures in the _toc.yml -- but it might be nice to be able to switch it off (if needed).

@AakashGfude
Copy link
Contributor Author

AakashGfude commented Mar 3, 2021

Hey all - in general this looks good to me! It's a bit unclear what all the tableofcontents changes are affecting (e.g. if it's just for latex or also for HTML).

It only affects the Latex output, the one which was written earlier for HTML has just been copied in this else case
https://github.com/executablebooks/jupyter-book/pull/1167/files#diff-d6d834bc60c86a22e11aed333d22d8ac5f9eb097f8da28d3c299da8e2806adf5R158

@mmcky mmcky self-requested a review March 9, 2021 03:53
@mmcky
Copy link
Contributor

mmcky commented Mar 9, 2021

@chrisjsewell @choldgraf I am planning to merge this on Friday 12th March unless there are any further comments or objections. Thanks!

@choldgraf
Copy link
Collaborator

+1 from me 👍

@mmcky
Copy link
Contributor

mmcky commented Mar 12, 2021

thanks @AakashGfude for this pull request and enhancements -- and iterating on comments and design. This will be a useful addition and big improvement to LaTeX builds.

@mmcky mmcky dismissed chrisjsewell’s stale review March 12, 2021 09:09

It looks like comment has been acted on but GitHub hasn't registered that.

@mmcky
Copy link
Contributor

mmcky commented Mar 12, 2021

@chrisjsewell I am not sure why GitHub still had your change request as open as @AakashGfude had implemented some changes around your comments. So I dismissed that review (as stale to get a green Squash and Merge Status). If there is anything I missed - we can put together an update / patch PR.

@mmcky mmcky merged commit a3065b1 into jupyter-book:master Mar 12, 2021
@mmcky mmcky deleted the latex-tocd branch March 12, 2021 09:11
@chrisjsewell
Copy link
Contributor

Heya @AakashGfude @mmcky, yeh in general all looks great thanks 👍
Two nitpicks though 😬:

also given it is part of the required installs, we need it to have a conda-forge package

I don't see a jupyter-latex package on conda yet? Can you address this as a priority

a3065b1; can you make sure that commit messages adhere to https://github.com/executablebooks/.github/blob/master/CONTRIBUTING.md#commit-messages

@mmcky
Copy link
Contributor

mmcky commented Mar 12, 2021

oh man that commit message is awful. Sorry @chrisjsewell. I should have adjusted from the first cell of the PR. Is there a way to amend this?

@mmcky
Copy link
Contributor

mmcky commented Mar 12, 2021

I don't see a jupyter-latex package on conda yet?

@chrisjsewell
Copy link
Contributor

can you make sure that commit messages adhere to executablebooks/.github@master/CONTRIBUTING.md#commit-messages

Same goes for @choldgraf with 5de7ca6 😉 it just helps for understanding changes in retrospect and constructing the changelog for a release

@chrisjsewell
Copy link
Contributor

I should have adjusted the first cell of the PR. Is there a way to amend this?

you can edit any of the message cells (top-right), but also you can just edit the message directly, before pressing "Confirm squash and merge"

@chrisjsewell
Copy link
Contributor

@AakashGfude can you organise this as a priority.

let me know if you need any help, its pretty straightforward

@mmcky
Copy link
Contributor

mmcky commented Mar 12, 2021

I should have adjusted the first cell of the PR. Is there a way to amend this?

you can edit any of the message cells (top-right), but also you can just edit the message directly, before pressing "Confirm squash and merge"

Sorry I meant is there a way to amend the commit message? I'll have a look over the first cell of the PR history too.

@chrisjsewell
Copy link
Contributor

you can do an interactive rebase of the master branch, although thats a bit naughty 😬 (I'm not sure if just changing the message changes the commit hash). Perhaps if you write the intended commit message here I can amend it.

@mmcky
Copy link
Contributor

mmcky commented Mar 12, 2021

oh that definitely sounds like something I'm not going to try :-).

The initial PR message has more details but here is a good summary


✨ NEW: Introducing jupyterbook-latex and added handling of tableofcontents for latex (PR #1167)

This PR brings improvements to PDF builds via LaTeX by incorporating the jupyterbook-latex extension. This initial release:

  1. harmonises the document structure for html and latex outputs, by implementing support for part/chapter structures in the _toc.yml for PDF via LaTeX,
  2. brings support for the jupyter-book directive tableofcontents to PDF via LaTeX, and
  3. adds support for commonly used tags on code-cell directives (such as hide-cell) to PDF via LaTeX.

chrisjsewell pushed a commit that referenced this pull request Mar 12, 2021
…tents for latex (PR #1167)

This PR brings improvements to `PDF` builds via `LaTeX` by incorporating the
[jupyterbook-latex](https://github.com/executablebooks/jupyterbook-latex) extension.

This initial release:

1. harmonises the document structure for `html` and `latex` outputs, by implementing support for `part`/`chapter` structures in the `_toc.yml` for PDF via LaTeX,
2. brings support for the jupyter-book directive `tableofcontents` to PDF via LaTeX, and
3. adds support for commonly used tags on `code-cell` directives (such as `hide-cell`) to PDF via LaTeX

Co-authored-by: mmcky <mamckay@gmail.com>
Co-authored-by: mmcky <mmcky@users.noreply.github.com>
@chrisjsewell
Copy link
Contributor

ok sorted, yeh you shouldn't normally rebase master, because it can screw up other peoples branches/PRs of it. But as this was only just merged, then hopefully no one would have updated their branches with it anyway

@AakashGfude
Copy link
Contributor Author

@AakashGfude can you organise this as a priority.

let me know if you need any help, its pretty straightforward

I have to send a PR of the recipe to https://github.com/conda-forge/staged-recipes . am I right?

@chrisjsewell
Copy link
Contributor

Yep 👍

@choldgraf
Copy link
Collaborator

choldgraf commented Mar 13, 2021

congrats @AakashGfude and @mmcky on getting this one in 🚀🚀🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants