GH-84435: Make pyspecific directives translatable

[cocoatomo]

https://bugs.python.org/issue40254

The original issue: python/python-docs-ja#24

The patch provided from tk0miya, the Sphinx main contributor:
https://gist.github.com/tk0miya/e2de1962f073d389251ba13e285b4336

https://bugs.python.org/issue40254

• Issue: pyspecific directives are not translatable #84435

[cocoatomo]

Would someone attach the "skip news" label to this PR?

[JulienPalard]

Looking at this (a bit late), would this change also remove the 100+ "Raises an :ref:auditing event..." from the .po files and add a single one with placeholders in sphinx.po?

[cocoatomo]

Excuse me for late. The aim of this PR is the same as the one you supposed.

[JulienPalard]

It's interesting the. I had a totally different approch in #24945

I don't remember if my approch works at deduplicating msgids in the po files though, I'm testing.

[JulienPalard]

Hum yes, inded my version still generates tons of:

c-api.pot:msgid "Raises an :ref:`auditing event <auditing>` ``setopencodehook`` with no arguments."
c-api.pot:msgid "Raises an :ref:`auditing event <auditing>` ``cpython._PySys_ClearAuditHooks`` with no arguments."
c-api.pot:msgid "Raises an :ref:`auditing event <auditing>` ``cpython.PyInterpreterState_New`` with no arguments."
c-api.pot:msgid "Raises an :ref:`auditing event <auditing>` ``cpython.PyInterpreterState_Clear`` with no arguments."
using.pot:msgid "Raises an :ref:`auditing event <auditing>` ``cpython.run_command`` with argument ``command``."
using.pot:msgid "Raises an :ref:`auditing event <auditing>` ``cpython.run_module`` with argument ``module-name``."
using.pot:msgid "Raises an :ref:`auditing event <auditing>` ``cpython.run_stdin`` with no arguments."
using.pot:msgid "Raises an :ref:`auditing event <auditing>` ``cpython.run_file`` with argument ``filename``."

so yes this is interesting!! Can you please rebase on top of main?

[JulienPalard]

Rebased it and tested it 👍

[JulienPalard]

@cocoatomo OK I understand now, and was able to fix it using a git commit --amend --autor ....

[JulienPalard]

@cocoatomo It looks OK to me for a merge. I just let you review if I rebased correctly and if I correctly amended the autor attribute, let me know.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[humitos]

It would be good to be able to include this PR in 3.12 -- this will help a lot the work of the translators 😄

conflicts resolved

[AA-Turner]

Nobody expects the Spanish Inquisition!

edit: Bedevere has been gotten by the Spanish Inquisition...

[hugovk]

Thanks all!

[miss-islington]

Thanks @cocoatomo for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

[bedevere-bot]

GH-107681 is a backport of this pull request to the 3.12 branch.

[bedevere-bot]

GH-107682 is a backport of this pull request to the 3.11 branch.

[CAM-Gerlach]

@AA-Turner @JulienPalard @humitos Sorry for the delay; just say this. But is there a reason for calling gettext on each individual label instead of just in run() as before once the appropriate label has been selected, aside from possibly a small amount of extra overhead?

In the patch this was adapted from, there was only a single label for this directive, but in this version there's multiple very similar ones, and AFAIK the second one isn't even actually used in practice currently. Therefore, translators now have to translate both, rather than just the latter as in the previous version.

I've been working on a heavily-overhauled version of this directive with (among other things) much more sophisticated structured label and content generation, which should greatly reduce if not eliminate (on many or even most deprecations) the need to individually translate the current mostly-repetitive manual text of each deprecation message (I plan to coordinate with the translation folks to test it and provide feedback before it gets merged so I can ensure everything goes smoothly for that).

Besides the merge conflict (which alerted me to this change), unlike the original, this modified approach poses a problem for even my relatively minimal initial changes to factor out the obvious duplication/repetition here (where the message is repeated twice, aside from the addition of two short words), much less scaling to the more complex changes, where I'll need to either call gettext individually on atomic clauses, or wrap them ininline(translatable=True) nodes.

Therefore, I'd like to understand more regarding the motivations and background for this specific change. Thanks!

[AA-Turner]

For extensions, _() ought be called on all static text. See https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-locale_dirs.

[CAM-Gerlach]

For extensions, _() ought be called on all static text.

Yeah; what I'm asking about is the reason for the change here—calling it separately on both (duplicative) string literals rather than the prior status quo of calling it once on the (still static) string actually picked (other than being possibly slightly more efficient at runtime).

As I describe, the original patch this was based on had only one label, so it made perfect sense to just call gettext immediately on the original string rather than inside run(), but with multiple (repetitive) labels doing so there rather than at run time results in needing to translate the second label that (AFAIK) is never actually used in our docs, and conflicts with factoring out the non-DRY duplication here.

For reference, here are the relevant bits of this version vs. the refactored one using the previous approach:

class DeprecatedRemoved(Directive):
    _deprecated_label = sphinx_gettext('Deprecated since version {deprecated}, will be removed in version {removed}')
    _removed_label = sphinx_gettext('Deprecated since version {deprecated}, removed in version {removed}')
    
    def run(self):
        ...
        if current_version < removed_version:
            label = self._deprecated_label
        else:
            label = self._removed_label
        text = label.format(deprecated=self.arguments[0], removed=self.arguments[1])   

class DeprecatedRemoved(SphinxDirective):
    _deprecated_label = "Deprecated since version {deprecated}"
    _removed_label = "removed in version {removed}"

    def _generate_label_text(...):
        ...
        will_be = "" if is_removed else "will be "
        label = sphinx_gettext(f"{self._deprecated_label}, {will_be}{self._removed_label}")
        text = label.format(deprecated=deprecated_arg, removed=removed_arg)

See https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-locale_dirs

I'm not really clear on how that config value relates to the present discussion on when to call gettext in a directive, sorry. Did you maybe link to the wrong section?

[merwok]

        label = sphinx_gettext(f"{self._deprecated_label}, {will_be}{self._removed_label}")

This line expects xgettext or similar extraction tools to build all six possible versions of the string? That can’t work