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

Revert "Disable localisation when SOURCE_DATE_EPOCH is set (#10949)" #11306

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Apr 9, 2023

Reverts pull request #10949 and follow-up commit 2c83af0.

(post-merge of that PR, I haven't been able to re-confirm the before-and-after behaviour of the fix, and I'd prefer not to risk it being included in a sphinx release and causing issues for users until we're fairly-highly confident that it fixes the problem, only the problem, and doesn't introduce unintended side-effects)

@jayaddison jayaddison force-pushed the issue-9778-revert/remove-source_date_epoch-null-translation branch from dde3e9f to 78a7c61 Compare April 9, 2023 16:31
@jayaddison
Copy link
Contributor Author

From further investigation: I do now believe that #10949 was unnecessary; see commentary at #9778 (comment) for details.

@@ -203,7 +203,12 @@ def setup(app):
.. versionadded:: 1.8
"""
def gettext(message: str) -> str:
return _TranslationProxy(catalog, namespace, message) # type: ignore[return-value]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed like a nice cleanup -- and unlocks other simplifications -- so if it's possible to re-apply later in a separate change, then I'd like to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Which bit were you talking about here? Open to keeping it if my partial revert didn't.

A

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular: switching everything over to lazy-loading (so that the gettext method doesn't require any conditional logic) - and as a side-effect, allowing content to be localised into more than one language during a unit test.

Referring back to some previous notes:

Attempting to write a test case to build the same application in two different languages was not initially possible -- the first-loaded translation catalog (as found in the sphinx.locale.translators global variable) -- would remain in-use for subsequent application builds under different locales

This can wait until after 6.2.x, unless you're particularly keen to dig into it. I'd plan to take another look at it fresh in a few days' time.

@AA-Turner AA-Turner closed this Apr 21, 2023
@jayaddison jayaddison deleted the issue-9778-revert/remove-source_date_epoch-null-translation branch April 21, 2023 21:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants