Allow enabling translations by setting `translations_list` in config #302

Eric-Arellano · 2023-05-05T19:17:37Z

Part of #262. Before, to add translation support, you would have to copy and paste versionutils.py. Now, you ~only need to define translations_list in conf.py and qiskit_sphinx_theme will set up the rest for you.

That duplication of versionutils.py was annoying to copy and also made it too easy to fall out of sync, resulting in #272.

To prove this works, this PR removes language_utils.py from example_docs/ and instead sets its values in conf.py. Translations still work:

This PR intentionally does not refactor any of the translations.py code. It only copies and pastes what we had from all our projects, to make it easier to review. The only divergences are:

Removing the translations config option and HTML context. It wasn't being used by anything.
Removing the current_translation HTML context. It wasn't being used by anything.

…ranslation`

Eric-Arellano · 2023-05-05T19:23:16Z

README.md

+
+* Ensure that `qiskit_sphinx_theme` is in the `extensions` setting.
+* Set the option `translations_list` to a list of pairs of the locale code and the language name, e.g. `[..., ("de_DE", "German")]`.
+* Set the option `content_prefix` to your project's URL prefix, like `ecosystem/finance`.


There are a couple of things I don't like about this option, including the name not being clear what it is: #303.

I plan to make some improvements as part of Sphinx Theme 0.12. But I want to do that in a follow up PR to make the code review easier.

For now, this simply copies the name and logic we had in all of our Terra and Ecosystem projects.

As long as we make any changes before 0.12 is released, it is safe for us to make breaking changes like renaming this.

Eric-Arellano · 2023-05-05T19:24:24Z

example_docs/docs/conf.py

@@ -41,7 +40,7 @@
    'jupyter_sphinx',
    'sphinx_design',
    "nbsphinx",
-    "sphinxcontrib.jquery",


I figured out now why jQuery wasn't being activated automatically by qiskit-sphinx-theme like I hoped for in #277. It's not enough to set html_theme = qiskit_sphinx_theme. We need to also add it to extensions.

so does this mean we don't need sphinxcontrib.jquery anymore?

If you set qiskit_sphinx_theme in extensions, then it is redundant to also include sphinxcontrib.jquery. That is because we have this code that tells our own extension to also activate jQuery:

qiskit_sphinx_theme/qiskit_sphinx_theme/__init__.py

Lines 35 to 37 in 086f163

# Sphinx 6 stopped including jQuery by default. Our Pytorch theme depend on jQuery,

# so install it for our users automatically.

app.setup_extension("sphinxcontrib.jquery")

Eric-Arellano · 2023-05-05T19:24:41Z

qiskit_sphinx_theme/__init__.py

+    else:
+        # Sphinx 6 stopped including jQuery by default. Our Pytorch theme depend on jQuery,
+        # so activate it for our users automatically.
+        app.setup_extension("sphinxcontrib.jquery")


No need to activate jQuery for Furo.

qiskit_sphinx_theme/translations.py

example_docs/docs/conf.py

SooluThomas · 2023-05-05T19:39:36Z

qiskit_sphinx_theme/translations.py



 def _extend_html_context(app, config):
    context = config.html_context
-    context['translations'] = config.translations
-    context['translations_list'] = translations_list
-    context['current_translation'] = _get_current_translation(config) or config.language


Do you know if context['current_translation'] is now used else where?

It was added, IIRC, for the page to remember the language the user was on, when they go from one page to another.

I couldn't find anywhere it's used. html_context exists so that HTML templates can reference values that are inserted by Python.

Our templates are using language_label instead. If you look at the Python implementation, language_label and current_translation are computed almost the same way.

I think it's used in qiskit: https://github.com/Qiskit/qiskit-metapackage/blob/a6a6eb885939ff26e59b77d1e22c8fca9b55ed9d/docs/versionutils.py#LL55C89-L55C89

It is indeed set by Qiskit and all the ecosystem projects. But it is never consumed by anything. We're setting a value for no benefit, which is confusing.

ah ok, let's remove it for now but make a mental note if we find something breaks for someones project we might need to add it back

Sounds good!

Eric-Arellano · 2023-05-08T13:47:33Z

This PR intentionally does not refactor any of the translations.py code.

I opened #305 to track adding unit tests to this file in a follow-up PR. I don't do that in this PR because we don't yet have the infrastructure to run Python unit tests and I wanted to keep the review simple.

example_docs/docs/conf.py

javabster · 2023-05-08T17:44:10Z

example_docs/docs/conf.py

@@ -41,7 +40,7 @@
    'jupyter_sphinx',
    'sphinx_design',
    "nbsphinx",
-    "sphinxcontrib.jquery",


so does this mean we don't need sphinxcontrib.jquery anymore?

example_docs/docs/conf.py

javabster · 2023-05-08T17:48:02Z

README.md

+
+Once the Translations team is ready, then update your `conf.py`:
+
+* Ensure that `qiskit_sphinx_theme` is in the `extensions` setting.


is this step only necessary for enabling translations? Or should it be in extensions anyway?

Sort of. If you don't add qiskit_sphinx_theme to extensions, then you need to add sphinxcontrib.jquery to the extensions list because Pytorch requires jQuery. At least until we switch to Furo.

So, I think we can simplify things to say that you should always add qiskit_sphinx_theme to the extensions list.

example_docs/docs/conf.py

javabster · 2023-05-08T17:53:01Z

qiskit_sphinx_theme/translations.py



 def _extend_html_context(app, config):
    context = config.html_context
-    context['translations'] = config.translations
-    context['translations_list'] = translations_list
-    context['current_translation'] = _get_current_translation(config) or config.language


I think it's used in qiskit: https://github.com/Qiskit/qiskit-metapackage/blob/a6a6eb885939ff26e59b77d1e22c8fca9b55ed9d/docs/versionutils.py#LL55C89-L55C89

javabster · 2023-05-08T17:58:14Z

qiskit_sphinx_theme/translations.py

have you cross checked this file with https://github.com/Qiskit/qiskit-metapackage/blob/master/docs/versionutils.py? I think there are a couple of functions used in qiskit that aren't here, e.g. _get_language_label and _get_translation_url. I'm not really sure what those functions do but it might be important if it's used in qiskit

Those are here too :) This is based on that file, and only remove the Git/Previous Versions code. Along with what I mention in the PR description about translations config value and current_translation.

Oh! I didn't expand the file view in the github diff 😅 my bad, I see them now

javabster

LGTM 🚀

Eric-Arellano added 5 commits May 5, 2023 12:27

Remove Git/VCS logic

525c497

Activate translations.py plugin

181f5e3

translations_list comes from config

483290b

Remove unused config value translations and html context `current_t…

16ecf10

…ranslation`

Add instructions

cec55f3

Eric-Arellano mentioned this pull request May 5, 2023

Determine what we want to do with content_prefix option #303

Closed

Eric-Arellano commented May 5, 2023

View reviewed changes

Eric-Arellano requested review from HuangJunye, javabster, coruscating and SooluThomas May 5, 2023 19:25

Eric-Arellano mentioned this pull request May 5, 2023

Language switcher doesn't work in Runtime docs and Nature docs #272

Closed

SooluThomas reviewed May 5, 2023

View reviewed changes

Eric-Arellano mentioned this pull request May 8, 2023

Add automated testing of translations.py #305

Closed

Eric-Arellano mentioned this pull request May 8, 2023

Generalize Previous Versions logic? #307

Open

javabster reviewed May 8, 2023

View reviewed changes

Eric-Arellano and others added 2 commits May 8, 2023 12:31

Review feedback: copyright, translations comment, README

d7cc253

Merge branch 'main' into language-plugin

fcb5c45

javabster approved these changes May 9, 2023

View reviewed changes

Merge branch 'main' into language-plugin

ebefcfa

Eric-Arellano merged commit 5dde71e into Qiskit:main May 9, 2023

Eric-Arellano deleted the language-plugin branch May 9, 2023 16:00

Eric-Arellano mentioned this pull request May 11, 2023

Include common files like autosummary templates and gallery.css in the theme itself #262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow enabling translations by setting `translations_list` in config #302

Allow enabling translations by setting `translations_list` in config #302

Eric-Arellano commented May 5, 2023

Eric-Arellano May 5, 2023

Eric-Arellano May 5, 2023

javabster May 8, 2023

Eric-Arellano May 8, 2023

Eric-Arellano May 5, 2023

SooluThomas May 5, 2023

Eric-Arellano May 8, 2023

javabster May 8, 2023

Eric-Arellano May 8, 2023

javabster May 9, 2023

Eric-Arellano May 9, 2023

Eric-Arellano commented May 8, 2023

javabster May 8, 2023

javabster May 8, 2023

Eric-Arellano May 8, 2023

javabster May 8, 2023

javabster May 8, 2023

Eric-Arellano May 8, 2023

javabster May 9, 2023

javabster left a comment

	# Sphinx 6 stopped including jQuery by default. Our Pytorch theme depend on jQuery,
	# so install it for our users automatically.
	app.setup_extension("sphinxcontrib.jquery")


		Once the Translations team is ready, then update your `conf.py`:

		* Ensure that `qiskit_sphinx_theme` is in the `extensions` setting.

Allow enabling translations by setting translations_list in config #302

Allow enabling translations by setting translations_list in config #302

Conversation

Eric-Arellano commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javabster left a comment

Choose a reason for hiding this comment

Allow enabling translations by setting `translations_list` in config #302

Allow enabling translations by setting `translations_list` in config #302