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

⬆️ UPGRADE: Upgrade to support sphinx4, remove support for sphinx2 #356

Merged
merged 7 commits into from
Sep 2, 2021

Conversation

mmcky
Copy link
Member

@mmcky mmcky commented Aug 30, 2021

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

fixes #338

Updated testing to:

  • test python=3.6 to python=3.9 against sphinx3 and sphinx4 release on ubuntu-latest
  • test python=3.9 and sphinx4 for osx-latest
  • test python=3.7and sphinx3 for windows-latest

This PR adds support for tracking differences between sphinx3 and sphinx4 but tracking of docutils versions in the test fixtures is not required at this stage.

@mmcky mmcky mentioned this pull request Aug 30, 2021
4 tasks
@mmcky
Copy link
Member Author

mmcky commented Aug 30, 2021

@chrisjsewell this PR has far fewer changes than #352 as it turns out there don't seem to be a huge number of sphinx specific differences in our fixture set for myst-nb. In fact only one test in test_glue.py::test_parser.

I have added a string though to conftests.py::sphinx_run to enable any future updates to either:

  1. what software we need to track in fixtures (i.e. perhaps docutils later on)
  2. a single reference point so they all match conventions.

@mmcky
Copy link
Member Author

mmcky commented Aug 30, 2021

@chrisjsewell @choldgraf I reconfigured github actions a little to test all python versions for both sphinx3 and sphinx4 with the one special case for windows.

I am wondering if we should:

  • conduct python=3.7,3.8,3.9 testing against sphinx4 only
  • add in python=3.9 and sphinx4 for os x

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #356 (9aecefb) into master (a3b2c6e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #356   +/-   ##
=======================================
  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...9aecefb. Read the comment docs.

@mmcky
Copy link
Member Author

mmcky commented Aug 30, 2021

@chrisjsewell not sure why

tests (ubuntu-latest, 3.6, >=3,<4) Expected — Waiting for status to be reported
tests (ubuntu-latest, 3.8, >=2,<3) Expected — Waiting for status to be reported

are expected given the .github/workflows/tests.yml

@@ -26,12 +26,9 @@ jobs:
fail-fast: false
matrix:
os: [ubuntu-latest]
python-version: [3.6, 3.7, 3.8, 3.9]
sphinx: [">=3,<4"]
python-version: [3.7, 3.8, 3.9]
Copy link
Member

Choose a reason for hiding this comment

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

If you are deprecating python 3.6 support, then you need to update setup.cfg

Copy link
Member

Choose a reason for hiding this comment

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

and also tox.ini

Copy link
Member Author

Choose a reason for hiding this comment

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

ah right -- roger that.

Perhaps I won't in the case. This can be done separately if we want to do that as a project.

Copy link
Member

Choose a reason for hiding this comment

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

the endo of life is in a few months, so it is fine to do so: https://devguide.python.org/#status-of-python-branches

Copy link
Member Author

@mmcky mmcky Aug 30, 2021

Choose a reason for hiding this comment

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

still not sure why

tests (ubuntu-latest, 3.8, >=2,<3) Expected — Waiting for status to be reported

is expected though. @chrisjsewell I can't see sphinx2 anywhere across the various configs.

Copy link
Member

Choose a reason for hiding this comment

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

it's part of the branch protection rules

Copy link
Member Author

Choose a reason for hiding this comment

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

@chrisjsewell that end of life chart is pretty great. 👍 I might open an issue to EOL python3.6 for December

Copy link
Member Author

@mmcky mmcky Aug 30, 2021

Choose a reason for hiding this comment

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

it's part of the branch protection rules

Oh -- thanks I would never have checked there :-)

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

mmcky commented Aug 30, 2021

OK so I have updated testing to:

  • test python=3.6 to python=3.9 against sphinx3 and sphinx4 release on ubuntu-latest
  • test python=3.9 and sphinx4 for osx-latest
  • test python=3.7and sphinx3 for windows-latest

@mmcky
Copy link
Member Author

mmcky commented Aug 30, 2021

@chrisjsewell do you want to be explicit about testing of docutils versions. In the current tox setup:

docutils=0.16 gets installed for py##-sphinx3 and
docutils=0.17 gets installed for py##-sphinx4

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 in general LGTM! As long as tests are happy then I think we should go for it.

@@ -0,0 +1,169 @@
<document source="with_glue">
Copy link
Member

Choose a reason for hiding this comment

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

Should we now expect a new one of these files for every major version of Sphinx that we support?

Copy link
Member Author

@mmcky mmcky Aug 30, 2021

Choose a reason for hiding this comment

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

@choldgraf the sphinx3 is the same as the old fixture file. There is a minor difference in what sphinx includes in the xml "align='default'" is no longer specified in sphinx4

@mmcky
Copy link
Member Author

mmcky commented Aug 30, 2021

@chrisjsewell do you want to cast an eye over this before it is merged?

@mmcky
Copy link
Member Author

mmcky commented Sep 1, 2021

thanks @choldgraf for reviewing this. I will merge this tomorrow if there are no further comments.

@choldgraf
Copy link
Member

In the meeting today I think that we agreed that it's time to merge this one and cut a release! 🚀

@mmcky
Copy link
Member Author

mmcky commented Sep 2, 2021

Absolutely - I can do that this afternoon. Just tied up with a few other things this morning.

@mmcky mmcky merged commit 9f6fae2 into master Sep 2, 2021
@mmcky mmcky deleted the support-sphinx4+ branch September 2, 2021 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow using Sphinx 4
3 participants