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

Make the tests pass with Sphinx 7.1 #178

Merged
merged 1 commit into from Sep 7, 2023

Conversation

mitya57
Copy link
Contributor

@mitya57 mitya57 commented Aug 5, 2023

The tests were failing with this diff:

-<document source="index.rst">
+<document source="index.rst" translation_progress="{'total': 0, 'translated': 0}">

See https://bugs.debian.org/1042589 for the full log.

This was caused by sphinx-doc/sphinx#11509, which is part of Sphinx ≥ 7.1.0.

@welcome
Copy link

welcome bot commented Aug 5, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@foster999
Copy link
Collaborator

Thanks for raising the PR ☺️ we've had previous requests for supporting translation, so I wanted to check whether this change will prevent this in future? We would generally try to support new sphinx features rather than disable them

@mitya57
Copy link
Contributor Author

mitya57 commented Aug 6, 2023

I disabled this feature only in tests, it won't be disabled for sphinx-tabs users.

When you decide to drop support for old Sphinx versions (< 7.1), you can revert this change and update the test expectations instead.

@foster999
Copy link
Collaborator

I think it would be best for the tests to reflect the latest sphinx version. We should be able to fix this by regenerating the regression tests under a normal test run

@mitya57
Copy link
Contributor Author

mitya57 commented Aug 6, 2023

But then the tests will fail with older Sphinx.

Although, I can put some .replace() in Python code to make it work with old Sphinx too. Will that work for you?

Update the expected XML files to match Sphinx 7.1 output, and patch
Sphinx < 7.1 output to match our new expectations.
@mitya57
Copy link
Contributor Author

mitya57 commented Aug 6, 2023

I have implemented the approach from my last comment now.

@mitya57 mitya57 changed the title Disable TranslationProgressTotaliser to fix tests with Sphinx 7.1 Make the tests pass with Sphinx 7.1 Aug 6, 2023
Copy link
Collaborator

@Daltz333 Daltz333 left a comment

Choose a reason for hiding this comment

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

LGTM

@Daltz333 Daltz333 merged commit f60a10e into executablebooks:master Sep 7, 2023
2 checks passed
@welcome
Copy link

welcome bot commented Sep 7, 2023

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

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.

None yet

3 participants