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

Split up logic in sphinx.transforms.i18n.Locale.apply #11166

Merged
merged 6 commits into from Mar 15, 2023

Conversation

jeanas
Copy link
Contributor

@jeanas jeanas commented Jan 31, 2023

Before, the apply method was one big 400-line chunk. This just splits it up into more manageable chunks by introducing a _NodeUpdater auxiliary class holding logic for updating various types of references.

Feature or Bugfix

  • Refactoring

Purpose

Make the Locale transform easier to understand and modify.

Before, the apply method was one big 400-line chunk. This just splits it
up into more manageable chunks by introducing a _NodeUpdater auxiliary
class holding logic for updating various types of references.
@jeanas
Copy link
Contributor Author

jeanas commented Jan 31, 2023

To give a bit of context, I did this as a prelude to trying my hand on actual functionality changes (like for #11157). I was finding the Locale transform pretty hard to follow.

@jeanas
Copy link
Contributor Author

jeanas commented Feb 15, 2023

Would anyone be able to review this PR? I understand that this is not necessarily the most fun type of PR, as it doesn't add anything by itself, but I have some planned i18n improvements that depend on it.

@m-aciek
Copy link
Contributor

m-aciek commented Mar 14, 2023

For some reason test_intl::test_gettext_dont_rebuild_mo test fails on Windows: https://github.com/sphinx-doc/sphinx/actions/runs/4180689136/jobs/7241911966.

>       assert get_number_of_update_targets(app0) == 0
E       AssertionError: assert 1 == 0
E        +  where 1 = <function test_gettext_dont_rebuild_mo.<locals>.get_number_of_update_targets at 0x000001F791A3B5F0>(<SphinxTestApp buildername='dummy'>)

@AA-Turner AA-Turner changed the title i18n: Split up Locale.apply logic Split up logic in sphinx.transforms.i18n.Locale.apply Mar 15, 2023
@AA-Turner AA-Turner merged commit b7345ad into sphinx-doc:master Mar 15, 2023
21 checks passed
@AA-Turner
Copy link
Member

Thank you @jeanas!

A

@jeanas jeanas deleted the intl-simpl branch March 15, 2023 22:54
@jeanas
Copy link
Contributor Author

jeanas commented Mar 15, 2023

Sorry for leaving those type errors! Thank you for taking care of them yourself, I wasn't expecting it from a busy maintainer.

The failed test seems flaky? I'm not sure why it failed, but it then passed on master.

@AA-Turner AA-Turner added this to the 6.2.0 milestone Apr 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 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

3 participants