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

MAIN: Enable support for Sphinx>=4 #352

Closed
wants to merge 17 commits into from
Closed

MAIN: Enable support for Sphinx>=4 #352

wants to merge 17 commits into from

Conversation

mmcky
Copy link
Member

@mmcky mmcky commented Aug 25, 2021

This PR enables support for sphinx>=4 and drops support for sphinx<=2

fixes #338

Tasks

  • rework tests to specify sphinx3 and sphinx4 for tests that differ (rather than all test cases)
  • Wait for ⬆️ UPGRADE: myst-parser to 0.15.2 #353 to be merged (which should actually be 0.15.2)
  • Rebase onto this branch
  • Fix changes needed just for Sphinx 4

This work has all been migrated to #356 so will close this PR. Thanks @chrisjsewell @choldgraf for comments and integration points with myst-parser

@mmcky mmcky marked this pull request as draft August 25, 2021 04:16
@mmcky
Copy link
Member Author

mmcky commented Aug 25, 2021

@chrisjsewell the current commit to this should setup a test environment through tox that includes sphinx4 but it doesn't seem to work for me. It keeps installing sphinx=3.5.4. Any ideas?

I widened the sphinx version requirement in setup.cfg and then added a sphinx4 deps section in the tox.ini

Then I tried to use tox -e py38-sphinx4

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #352 (093f2b4) into master (a3b2c6e) will not change coverage.
The diff coverage is n/a.

❗ Current head 093f2b4 differs from pull request most recent head 109c7b7. Consider uploading reports for the commit 109c7b7 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #352   +/-   ##
=======================================
  Coverage   87.43%   87.43%           
=======================================
  Files          12       12           
  Lines        1337     1337           
=======================================
  Hits         1169     1169           
  Misses        168      168           
Flag Coverage Δ
pytests 87.43% <ø> (ø)

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


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 a3b2c6e...109c7b7. Read the comment docs.

@chrisjsewell
Copy link
Member

myst-parser 0.14 does not support sphinx v4:https://github.com/executablebooks/MyST-Parser/blob/master/CHANGELOG.md#0150---2021-06-13

@mmcky
Copy link
Member Author

mmcky commented Aug 25, 2021

myst-parser 0.14 does not support sphinx v4:https://github.com/executablebooks/MyST-Parser/blob/master/CHANGELOG.md#0150---2021-06-13

Ah of course. Thanks @chrisjsewell. I'll pin to myst-parser=0.15. Thank you!

@chrisjsewell
Copy link
Member

Pin myst-parser~=0.15.1 👍

@chrisjsewell
Copy link
Member

Note: when upgrading jupyter-book with this new version, it will be going from myst-parser v0.13 -> v0.15.
There are a number of additions to configurations to consider: https://github.com/executablebooks/MyST-Parser/blob/master/CHANGELOG.md#0150---2021-06-13

@mmcky
Copy link
Member Author

mmcky commented Aug 25, 2021

So myst-parser==0.15.1 seems responsible for the changes in fixture outputs from https://github.com/executablebooks/MyST-Parser/blob/master/CHANGELOG.md#changed-mathjax-handling-%EF%B8%8F @chrisjsewell from your work on myst-parser is tracking major sphinx versions is still going to be needed going forward right?

If you look at diff for: tests/test_parser/test_complex_outputs.sphinx3.xml

those outputs were built with sphinx3 but the tox environment moved up to myst-parser=0.15.1 and docutils~=0.16 while the current test set builds with myst-parser=0.14

I guess I am wondering if I should:

or if we should just wrap all these version updates and changes in the one PR.

setup.cfg Outdated Show resolved Hide resolved
@mmcky
Copy link
Member Author

mmcky commented Aug 26, 2021

@chrisjsewell So far many of the fixtures seem to be the same between sphinx3 and sphinx4 so did you apply sphinx3, sphinx4 between fixtures for the ones that varied / or did you apply the identifier for all xml fixtures that document the doctree.pformat() -- just want to apply the same pattern here.

@chrisjsewell
Copy link
Member

between fixtures for the ones that varied / or did you apply the identifier for all xml fixtures

yeh just the ones that differed

@choldgraf
Copy link
Member

I've updated the top comment with what I think needs to happen in order to get this PR in - @mmcky let me know if I am off there! I find it easier to track "next steps" via the top comment rather than embedded in issues, because then you tend to get multiple to-do lists in different places in the responses.

@mmcky
Copy link
Member Author

mmcky commented Aug 27, 2021

@choldgraf sure thing 👍 -- this was mainly a branch I had opened to get some advice from @chrisjsewell but that makes a lot of sense.

@mmcky
Copy link
Member Author

mmcky commented Aug 30, 2021

Closing this as work is now contained in #356

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

Successfully merging this pull request may close these issues.

Allow using Sphinx 4
3 participants