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

Correctly support custom gettext output templates #12645

Merged

Conversation

jmbowman
Copy link
Contributor

Subject: Correctly support custom gettext output templates

Feature or Bugfix

  • Bugfix

Purpose

Resolves #10832 by allowing message.pot.jinja to be overridden by a custom template in the configured templates path. This allows output customization to be done once in the template rather than manually for each file every time the set of strings to be translated is updated.

Detail

  • To satisfy mypy, the type of the template_path parameter in GettextRenderer had to be updated to match that of its parent class.

Relates

@jmbowman jmbowman force-pushed the support_custom_gettext_templates branch from d95a925 to 75a2ed9 Compare July 22, 2024 14:18
@AA-Turner AA-Turner added this to the 8.x milestone Jul 23, 2024
Copy link
Contributor

@jayaddison jayaddison left a comment

Choose a reason for hiding this comment

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

This generally seems good to me - it allows projects to customize their gettext POT output when templates_path is configured for the project (by copying/placing and updating an appropriate message.pot.jinja file in one of those directories).

The difference-in-behaviour for gettext handling of templates_path -- the fact it will fallback automatically to the built-in Sphinx message.pot.jinja template -- I think is acceptable. Without that behaviour, many gettext builds would begin failing due to missing message.pot.jinja files.

I'll review again in a few days while thinking about any other side-effects and/or migration considerations.

@jayaddison
Copy link
Contributor

I'm still OK with these changes as-is; no additional thoughts/concerns in the past couple of days.

cc @n-peugnet in case you have any feedback, from your experience with i18n topics?

@n-peugnet
Copy link
Contributor

cc @n-peugnet in case you have any feedback, from your experience with i18n topics?

I don't have much experience with this specific part (the gettext extractor), but this is a useful feature that I already wished I could use.
The code, docs and test seems OK to me.

@AA-Turner AA-Turner merged commit 0cbdd98 into sphinx-doc:master Aug 11, 2024
20 checks passed
@jayaddison
Copy link
Contributor

👍

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2024
@AA-Turner AA-Turner modified the milestones: 8.x, 8.1.0 Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local GNU gettext template (message.pot.jinja) not used
4 participants