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

Remove Sphinx<5 compatibility leftovers #1512

Merged
merged 16 commits into from
Jan 3, 2024
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Aug 23, 2023

  • Remove blocks with logic for Sphinx<5
  • Always use writer-html5 CSS class
  • Use css_tag for better compatibility with newer Sphinx versions

@agjohnson this PR is a good candidate to be included in 2.0rc2 👍🏼 . If you approve it, I can try to make a release myself.

Since 2.0 we do not support Sphinx<5.
Since 2.0 HTML4 writer is not supported.
Keep `sphinx_version_info`. It will be required.

This reverts commit 3afbed2.
@humitos humitos requested a review from a team as a code owner August 23, 2023 12:55
Allows developer to run

```
tox -e py310-sphinx72-firefox
```

This will run the tests and open a Firefox after building the demo site
showing the `demo/demo.html` page on it.

Then, you can also run:

```
tox -e py310-sphinx61-firefox
```

and compare the visual differences.

Idea copied from #1388
Closes #1388
@humitos
Copy link
Member Author

humitos commented Aug 23, 2023

I tested this visually as well with Sphinx 5, 6 and 7 and I didn't find any issue so far.

@humitos humitos self-assigned this Aug 23, 2023
@humitos humitos added this to the 2.0 milestone Aug 25, 2023
Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

The changes here all seem reasonable, however I'd probably say we should plan out release structure a bit more. At very least, it makes sense to focus 2.0rc1 on just changing dependencies, not changing code yet. Then at least projects can upgrade dependencies and transitive dependencies without us changing anything major about their project output.

So for that, I'd probably say a 2.0rc1 with dependencies upgraded and a 2.1rc1 following with actual changes to our theme. In that case, we'd block this for a bit longer.

If we need any of these changes specifically for Sphinx 7 support, they can be broken away from some of the cleanup changes here. But this looks mostly like housekeeping.

@humitos humitos changed the title Remove Sphinx<5 compatibility and better support for newer versions Remove Sphinx<5 compatibility leftovers Aug 29, 2023
@humitos
Copy link
Member Author

humitos commented Aug 29, 2023

@agjohnson I split the required changes to support Sphinx >= 7.2 into #1519 and merge that one. I also update this PR with the latest changes.

Now, this PR is only and cleanup of the support for older (now) un-supported versions.

@agjohnson agjohnson modified the milestones: 2.0, 2.1 Aug 31, 2023
@agjohnson
Copy link
Collaborator

With 2.0 released, I think we're ready to merge this and prepare a 2.1a release. We haven't had any more feedback on the CSS changes in the 2.0rc{1,2,3,4} releases, so I think we're safe to continue on.

@humitos thoughts here?

@humitos
Copy link
Member Author

humitos commented Nov 30, 2023

Sounds good to me! I've updated the PR to resolve the conflicts.

@humitos
Copy link
Member Author

humitos commented Jan 2, 2024

This PR is ready to re-review and merge. I kept the injection of css_files code as it's on master which is working properly. On the other hand, I added a small comment for extra_css_files so we don't get confused again with that.

@humitos humitos requested a review from agjohnson January 2, 2024 11:48
@humitos humitos merged commit 80de90a into master Jan 3, 2024
8 checks passed
@humitos humitos deleted the humitos/sphinx-compatibility branch January 3, 2024 11:19
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.

arbitrary add_css_file attributes are not supported
4 participants