executablebooks / sphinx-tabs

Tabbed views for Sphinx
https://sphinx-tabs.readthedocs.io
MIT License
265 stars 68 forks source link

Sphinx 4 Support #110

Closed Daltz333 closed 3 years ago

codecov[bot] commented 3 years ago

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 commented 3 years ago

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

foster999 commented 3 years ago

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 commented 3 years ago

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

foster999 commented 3 years ago

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 commented 3 years ago

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

Daltz333 commented 3 years ago

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

Daltz333 commented 3 years ago

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

Daltz333 commented 3 years ago

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.