Skip to content

Sphinx 7 support#67

Closed
LecrisUT wants to merge 4 commits intoexecutablebooks:mainfrom
LecrisUT:sphinx-7
Closed

Sphinx 7 support#67
LecrisUT wants to merge 4 commits intoexecutablebooks:mainfrom
LecrisUT:sphinx-7

Conversation

@LecrisUT
Copy link
Copy Markdown
Contributor

@LecrisUT LecrisUT commented Jun 18, 2023

This one cannot be built because of dependency hell in the testing dependencies. Any advice?

See my fork pr for action runner LecrisUT#2

@choldgraf
Copy link
Copy Markdown
Member

choldgraf commented Jun 18, 2023

Given that this package is relatively simple and likely will not test the Sphinx API too much, I'd suggest we remove the upper bound pin on Sphinx in general. I'd suggest that we open an issue to track this decision, and if after one or two more major releases we have had problems associated with API changes that would have been caught by a major version pin, we can change the policy back. Does that make sense?

In this PR i suggest we remove the upper pin, and just add the tests for the new version. We can also remove the sphinx 5 and 6 tests accordingly so that we are just testing the newest and oldest supported releases

@LecrisUT
Copy link
Copy Markdown
Contributor Author

I already removed the upper pin in the setup.py file. Yes, we can remove the 5,6 versioned requirements as soon as a simple test passes. But I had install failures there right now. Let me fix the CI so I can show. Also do you want to track future releases as well in the CI?

@LecrisUT
Copy link
Copy Markdown
Contributor Author

LecrisUT commented Jun 18, 2023

Oh sorry all the actions pass. I think I have this confused with another package that broke some support 🤔. I will remove the intermediate sphinx requirements and this can be merged I think. Oops, I forgot to actually pin sphinx version. This one does have the dependency issues.

@LecrisUT LecrisUT force-pushed the sphinx-7 branch 4 times, most recently from ae5fef6 to ea80722 Compare June 18, 2023 08:25
@LecrisUT
Copy link
Copy Markdown
Contributor Author

Also, there's a chicken-and-egg problem with sphinx-book-theme to support sphinx 7. Should I add a temporary commit pointing it to my branch there to check the CI here?

@choldgraf
Copy link
Copy Markdown
Member

hmmm - I feel it'd be better if we actually didn't use the book theme in the testing suite since it's got some unnecessary complexity that we probably aren't testing with anyway. What if we either changed the theme to Alabaster on the fly in the test, or created a little dummy test site to run that didn't have the book theme dependencies.

@LecrisUT
Copy link
Copy Markdown
Contributor Author

Let me give it a quick try to see if we can just use the default/builtin themes. Hopefully they are not hard-coded in the tests

@LecrisUT
Copy link
Copy Markdown
Contributor Author

LecrisUT commented Jun 30, 2023

I don't understand the tests well enough to figure out what would be a minimum testing suite. Current version complains as:

Warning, treated as error:
role 'sub-ref' is already registered, it will be overridden

It seems that the minimum test dependencies are myst-parser and myst-nb because:

    # Check for MyST-NB cell structure to make sure it stays the same
    # If this breaks, we'll need to update our default cell selectors
    soup_nb = BeautifulSoup(
        Path(sphinx_build.path_pg_ntbk).read_text(), "html.parser"
    )

Luckily I don't see anything hard-coded about the theme or anything, so as soon as a minimum documentation can be built, it should be fine.

@kloczek
Copy link
Copy Markdown

kloczek commented Oct 26, 2023

Any chance to update this PR? 🤔

@LecrisUT
Copy link
Copy Markdown
Contributor Author

I need help with creating minimal tests, I am not familiar with the project to make the necessary mwe

@choldgraf choldgraf closed this Oct 28, 2023
@choldgraf choldgraf reopened this Oct 28, 2023
@LecrisUT
Copy link
Copy Markdown
Contributor Author

I'll open a new one for CI stuff

@LecrisUT LecrisUT closed this Oct 29, 2023
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.

3 participants