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

IMPROVE: Support Sphinx v4 #1448

Merged
merged 32 commits into from
Oct 14, 2021
Merged

IMPROVE: Support Sphinx v4 #1448

merged 32 commits into from
Oct 14, 2021

Conversation

mmcky
Copy link
Member

@mmcky mmcky commented Sep 2, 2021

This PR is to upgrade the jupyter-book stack for compatibility with sphinx>=4,<5

Tasks

WARNING: mathjax_config/mathjax2_config does not work for the current MathJax version, use mathjax3_config instead

Additional items identified by @chrisjsewell

Docs:

Fixes:

fixes #1133
fixes #1438
fixes #1181

@mmcky mmcky marked this pull request as draft September 2, 2021 07:31
@mmcky
Copy link
Member Author

mmcky commented Sep 2, 2021

@mmcky
Copy link
Member Author

mmcky commented Sep 7, 2021

  • sphinx 4 support required in sphinx-jupyterbook-latex
Warning, treated as error:
E         [jb-latex]: myst-nb version not compatible with >=0.11,<0.13: 0.13.0

@choldgraf
Copy link
Member

choldgraf commented Sep 28, 2021

hey @mmcky - what's the status on this one? Anything we need to do to unblock? It looks like the only remaining piece is the jupyterbook-latex one. I've added that as a task list to the top comment

@mmcky
Copy link
Member Author

mmcky commented Sep 28, 2021

@choldgraf working our way through the stack to update the relevant packages. Currently on sphinx-jupyterbook-latex then I think we are pretty close. I have had to rework some of the tests in sphinx-jupyterbook-latex so they didn't depend on jupyter-book which is now technically upstream.

@mmcky
Copy link
Member Author

mmcky commented Sep 29, 2021

Current build warnings

WARNING: cannot override config setting 'html_add_permalinks' with unsupported type, ignoring

this must be new coming in from master. Since sphinx>=3.5 this option is no longer supported and replaced by html_permalinks with a default value of True so I will remove html_add_permalinks from tests/test_config/* and default config.py`.

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #1448 (199d086) into master (a0d719f) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1448      +/-   ##
==========================================
+ Coverage   91.09%   91.17%   +0.07%     
==========================================
  Files           7        7              
  Lines         674      680       +6     
==========================================
+ Hits          614      620       +6     
  Misses         60       60              
Flag Coverage Δ
pytests 91.17% <100.00%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
jupyter_book/config.py 93.75% <100.00%> (+0.22%) ⬆️

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 a0d719f...199d086. Read the comment docs.

@mmcky
Copy link
Member Author

mmcky commented Sep 29, 2021

sphinx 4 will close MathJax version 3 support #963, but at the same time user who were previously setting mathjax_config should be warned that they now need to set mathjax2_config/mathjax3_config https://www.sphinx-doc.org/en/master/usage/extensions/math.html#confval-mathjax3_config

I have added 3f9bf18 to address this issue. It will issue a warning if a user is still using mathjax_config and sphinx>=4.0. If the user has specified v2 of mathjax via mathjax_path it will switch this warning off.

If the warning is issued then it will also migrate mathjax_config to mathjax3_config to enable projects to build.

@chrisjsewell @choldgraf what do you think of this solution? From using cdn addresses do you think:

"mathjax@2" in user_yaml_update["mathjax_path"]

is robust enough? All the mathjax_path address strings I have seen seem to contain mathjax@2 in them.

@chrisjsewell chrisjsewell linked an issue Oct 3, 2021 that may be closed by this pull request
@chrisjsewell
Copy link
Member

chrisjsewell commented Oct 3, 2021

another TODO for you 😉:

setup.cfg Outdated Show resolved Hide resolved
choldgraf
choldgraf previously approved these changes Oct 12, 2021
Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me - I think it's OK if the Sphinx config pattern changes in a subsequent PR, since right now that config is only exposed via the sphinx: key, which is more of a power-user feature anyway.

Co-authored-by: Chris Holdgraf <choldgraf@berkeley.edu>
@mmcky
Copy link
Member Author

mmcky commented Oct 13, 2021

thanks @choldgraf

@chrisjsewell I'd like LGTM, so let me know if you plan to review and I will hold off until you can do so.
Otherwise I'll merge on Friday.

Copy link
Member

@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.

A quick review thanks @mmcky

docs/content/code-outputs.md Outdated Show resolved Hide resolved
docs/content/code-outputs.md Outdated Show resolved Hide resolved
jupyter_book/config.py Show resolved Hide resolved
jupyter_book/default_config.yml Show resolved Hide resolved
@mmcky mmcky added this to the v0.12.0 milestone Oct 13, 2021
@mmcky
Copy link
Member Author

mmcky commented Oct 13, 2021

@chrisjsewell should

myst_dmath_double_inline: true  # Allow display math ($$) within an inline context

in default_config.yml be added to the schema file? Not exactly sure how the schema works.

@chrisjsewell
Copy link
Member

be added to the schema file?

yes, it should assert the type to be boolean

@mmcky
Copy link
Member Author

mmcky commented Oct 13, 2021

be added to the schema file?

yes, it should assert the type to be boolean

done in 9a6b5e6

@mmcky
Copy link
Member Author

mmcky commented Oct 13, 2021

thanks @chrisjsewell I think I have addressed your comments now 🎉

Copy link
Member

@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.

cheers

@mmcky
Copy link
Member Author

mmcky commented Oct 13, 2021

looks like windows based actions are timing out. I will re-run this in the morning to get the green tick of approval then I'll merge. Thanks everyone for your reviews.

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

I think that this one is good to go once tests are happy. I added an extra label to executablebooks/MyST-NB#365 so that the cross-ref works here too. I think we can merge this one once executablebooks/MyST-NB#365 is merged so that the docs work properly :-)

@choldgraf choldgraf mentioned this pull request Oct 13, 2021
4 tasks
@mmcky
Copy link
Member Author

mmcky commented Oct 14, 2021

@mmcky
Copy link
Member Author

mmcky commented Oct 14, 2021

thanks @chrisjsewell and @choldgraf for reviews. Greatly appreciated.

@mmcky mmcky merged commit 03583b9 into master Oct 14, 2021
Activity Board automation moved this from Review 👀 to Done Oct 14, 2021
@mmcky mmcky deleted the upgrade-sphinx4 branch October 14, 2021 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done 🎉
3 participants