Skip to content

GH-84435: Make pyspecific directives translatable #19470

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 6, 2023
Merged

GH-84435: Make pyspecific directives translatable #19470

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b13ef53
Have pyspecific directives translatable
cocoatomo Apr 11, 2020
c6e2870
Merge branch 'main' into translatable-pyspecific-directive
JelleZijlstra May 23, 2022
97ac866
Merge branch 'main' into translatable-pyspecific-directive
AA-Turner Aug 6, 2023
7f78726
De-duplicate translations
AA-Turner Aug 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions Doc/tools/extensions/pyspecific.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,13 @@ class ImplementationDetail(Directive):
final_argument_whitespace = True

# This text is copied to templates/dummy.html
label_text = 'CPython implementation detail:'
label_text = sphinx_gettext('CPython implementation detail:')

def run(self):
self.assert_has_content()
pnode = nodes.compound(classes=['impl-detail'])
label = sphinx_gettext(self.label_text)
content = self.content
add_text = nodes.strong(label, label)
add_text = nodes.strong(self.label_text, self.label_text)
self.state.nested_parse(content, self.content_offset, pnode)
content = nodes.inline(pnode[0].rawsource, translatable=True)
content.source = pnode[0].source
Expand Down Expand Up @@ -234,9 +233,9 @@ class AuditEvent(Directive):
final_argument_whitespace = True

_label = [
"Raises an :ref:`auditing event <auditing>` {name} with no arguments.",
"Raises an :ref:`auditing event <auditing>` {name} with argument {args}.",
"Raises an :ref:`auditing event <auditing>` {name} with arguments {args}.",
sphinx_gettext("Raises an :ref:`auditing event <auditing>` {name} with no arguments."),
sphinx_gettext("Raises an :ref:`auditing event <auditing>` {name} with argument {args}."),
sphinx_gettext("Raises an :ref:`auditing event <auditing>` {name} with arguments {args}."),
]

@property
Expand All @@ -252,7 +251,7 @@ def run(self):
else:
args = []

label = sphinx_gettext(self._label[min(2, len(args))])
label = self._label[min(2, len(args))]
text = label.format(name="``{}``".format(name),
args=", ".join("``{}``".format(a) for a in args if a))

Expand Down Expand Up @@ -414,8 +413,8 @@ class DeprecatedRemoved(Directive):
final_argument_whitespace = True
option_spec = {}

_deprecated_label = 'Deprecated since version {deprecated}, will be removed in version {removed}'
_removed_label = 'Deprecated since version {deprecated}, removed in version {removed}'
_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}')
Comment on lines +416 to +417
Copy link
Member

Choose a reason for hiding this comment

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

@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!

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@CAM-Gerlach CAM-Gerlach Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

        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


def run(self):
node = addnodes.versionmodified()
Expand All @@ -431,7 +430,6 @@ def run(self):
else:
label = self._removed_label

label = sphinx_gettext(label)
text = label.format(deprecated=self.arguments[0], removed=self.arguments[1])
if len(self.arguments) == 3:
inodes, messages = self.state.inline_text(self.arguments[2],
Expand Down