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

Fix loading custom template translations for en #12220

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

n-peugnet
Copy link
Contributor

@n-peugnet n-peugnet commented Apr 1, 2024

The English language was hardcoded to load empty translations for templates as part of #10067, but the rest of the code is already capable of handling missing translations just fine, so we can simply drop the first if block.

This allows to use translations for English in custom templates, as for all the other languages.

Fixes #12214

(the diff is better looked at when hiding whitespace changes)

The English language was hardcoded to load empty translations for
templates as part of sphinx-doc#10067,
but the rest of the code is already capable of handling missing
translations just fine, so we can simply drop the first if block.

This allows to use translations for English in custom templates, as for
all the other languages.

Fixes sphinx-doc#12214
@picnixz
Copy link
Member

picnixz commented Apr 1, 2024

Should I understand that, currently, we just assumed that locale.init would automatically handle everything while it was not the case? Like, it would work with Sphinx only because we are using our own internal templates & localizations, but would not for external ones because they could have other localized strings that we do not know about?

@n-peugnet
Copy link
Contributor Author

What I understand is that before #10067, the translator was initiated with an empty set when the language was set to None which was the default at that time. I this pull request, the default changed to en, and this if block was updated to reflect that, but without accounting for the custom templates, when the base language is not English. IMO this if can simply be dropped as I don't see why English should have a special exception, since it is not always the base language of a documentation.

@picnixz
Copy link
Member

picnixz commented Apr 1, 2024

I see. What I would like to avoid however is to load all the catalogs (which could be expensive) if there is no reason to do that. Is there a way to known whether template localization or not? if so, it would be good to check whether the translator needs to be initialized or not.

@n-peugnet
Copy link
Contributor Author

n-peugnet commented Apr 1, 2024

I'm not sure what you mean about "all the catalogs", this would only load le catalogs of the current language, if there are .po files for it. This change only remove an exception for the English language, and treats it as all the other ones.
As I understand it, if no user catalogs are defined for English, then there is no difference with the previous behaviour.

Here is an example build in a test project without any .po files for English:

Removing everything under '_build'...
Running Sphinx v7.3.0+/d5baa46d8
loading translations [en]... not available for built-in messages
making output directory... done
building [mo]: targets for 0 po files that are out of date
writing output... 
building [html]: targets for 1 source files that are out of date
updating environment: [new config] 1 added, 0 changed, 0 removed
reading sources... [100%] index
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
copying assets... copying static files... done
copying extra files... done
done
writing output... [100%] index
generating indices... genindex done
writing additional pages... search done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded.

We can see this message:

loading translations [en]... not available for built-in messages

While I agree that this message could now be changed or removed, we can see that no translation catalogs have been loaded.

And if a .po file exists for English, then it is more likely there for being used than for nothing.

@picnixz
Copy link
Member

picnixz commented Apr 1, 2024

Ah, I'm sorry by saying "all catalogs". Actually, since I never used localization, I'm just wondering this could make the build much slower or not (maybe I should just have said that I was worried about the complexity overhead). If the build time does not increase, I think this PR is fine (but I would like someone else to review it since I'm not well-versed in this topic).

Indeed, English is the source language of the Sphinx project, so there
is no catalogs for it. Wee still need to keep the catalog loading for
English, in case some translated strings are defined in custom templates
in project which source language is not English.
@n-peugnet
Copy link
Contributor Author

n-peugnet commented Apr 4, 2024

I just added a commit that always show "done" for English language, as the "not available for built-in messages" message could be misleading. Now the only diff in the log is the addition of the message "loading translations [en]... done":

--- before	2024-04-04 19:37:35.143948516 +0200
+++ after	2024-04-04 19:37:44.851948130 +0200
@@ -1,4 +1,5 @@
-Running Sphinx v7.2.6
+Running Sphinx v7.3.0+/bc127a0bf
+loading translations [en]... done
 making output directory... done
 building [mo]: targets for 0 po files that are out of date
 writing output... 

This should not change anything for most of the users, Sphinx will simply look for English translations which most of the time will not be existing. So it will load an empty translation catalog.
But this allows to load user defined English translations, if present, which in turn fixes #12214

P.S. This message is now only printed for languages that do indeed not have any translations available for built-in messages:

$ make html O=-Dlanguage=wtf
Running Sphinx v7.3.0+/bc127a0bf
loading translations [wtf]... not available for built-in messages

@picnixz
Copy link
Member

picnixz commented Apr 4, 2024

This should not change anything for most of the users, Sphinx will simply look for English translations which most of the time will not be existing. So it will load an empty translation catalog.

If you want to be even more precise, you could also check whether the catalog is empty or not (is it even possible?) and say that the catalog is empty. But this would be just nitpicking and I think people just don't care about that.

@picnixz
Copy link
Member

picnixz commented Apr 4, 2024

I'll let @chrisjsewell have a final look since I've never dealt with translations in general but it seems fine to me.

@chrisjsewell chrisjsewell self-requested a review April 4, 2024 17:49
@n-peugnet
Copy link
Contributor Author

I just fixed the merge conflict.

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.

Template translations (sphinx.po) are not used for English language
2 participants