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

Sphinx 4 Support #110

Merged
merged 10 commits into from May 3, 2021
Merged

Sphinx 4 Support #110

merged 10 commits into from May 3, 2021

Conversation

Daltz333
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Apr 11, 2021

Codecov Report

Merging #110 (3bf8111) into master (506e69f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #110   +/-   ##
=======================================
  Coverage   97.16%   97.16%           
=======================================
  Files           2        2           
  Lines         212      212           
=======================================
  Hits          206      206           
  Misses          6        6           
Flag Coverage Δ
pytests 97.16% <ø> (ø)

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 506e69f...3bf8111. Read the comment docs.

@Daltz333 Daltz333 requested a review from foster999 April 11, 2021 17:39
@Daltz333
Copy link
Collaborator Author

Daltz333 commented Apr 11, 2021

@foster999 could you take a look at the incompatibility with Sphinx 4? Feel free to push to my fork

@foster999
Copy link
Collaborator

Thanks for this @Daltz333

I've compared outputs between sphinx 3.5.4 and 4 (pre-release). Looks like the formatting of code-block's with line numbers has changed from:
image

to:

image

This seems to be an intentional change to improve accessibility (sphinx-doc/sphinx#7849), so I'll push a new sphinx-version-dependent test for this.

@Daltz333
Copy link
Collaborator Author

Daltz333 commented Apr 11, 2021

We may want to default html_codeblock_linenos_style to the old style for the user or expose with code-tab

@foster999
Copy link
Collaborator

Good point!

I've just tried setting the default to the old style in sphinx-tabs, but it seems like users are then unable to override this in their sphinx config.

Looking at the sphinx changelog, they're depreciating the html_codeblock_linenos_style option too. Given this, and that the new format improves accessibility, I'd be more inclined to follow their lead. We could document that users should use the sphinx option to revert the style (or stick with sphinx 3) and/or bump to a new major version to reflect that sphinx 4 causes a non-backwards compatible change?

@Daltz333
Copy link
Collaborator Author

Daltz333 commented Apr 11, 2021

Yeah, didn't realize they are deprecating the "table" option. I would do a major version bump for it.

@Daltz333
Copy link
Collaborator Author

I've pushed my changes, but I'm not sure how to conditionally skip the regression test

@Daltz333
Copy link
Collaborator Author

Okay, got things figured out.

Duplicated the other_assets test to include the Sphinx 4 generated output. They are conditionally skipped depending on sphinx version.

We probably shouldn't merge this until Sphinx 4 gets a stable release, that way we don't need to pin 4.0.0b1

Copy link
Collaborator

@foster999 foster999 left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Agree that holding on for a more stable release might be best. Sounds like it might only be the end of the month

setup.py Outdated Show resolved Hide resolved
Co-authored-by: Pradyun Gedam <3275593+pradyunsg@users.noreply.github.com>
@Daltz333
Copy link
Collaborator Author

Daltz333 commented May 3, 2021

Release is only 5 days away. I'm going to go ahead and merge it, and we can test Sphinx 4 major/minor versions in a later PR.

@Daltz333 Daltz333 merged commit e7544a6 into executablebooks:master May 3, 2021
@Daltz333 Daltz333 deleted the sphinx-4 branch May 3, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants