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

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

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:

classDeprecatedRemoved(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}') defrun(self): ... ifcurrent_version<removed_version: label=self._deprecated_labelelse: label=self._removed_labeltext=label.format(deprecated=self.arguments[0], removed=self.arguments[1]) 

classDeprecatedRemoved(SphinxDirective): _deprecated_label="Deprecated since version{deprecated}"_removed_label="removed in version{removed}"def_generate_label_text(...): ... will_be=""ifis_removedelse"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?

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