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

sphinx_immaterial.google_fonts._MAX_CONCURRENT_FETCHES variable is unused #351

Closed
duncanmmacleod opened this issue May 14, 2024 · 2 comments · Fixed by #363
Closed

sphinx_immaterial.google_fonts._MAX_CONCURRENT_FETCHES variable is unused #351

duncanmmacleod opened this issue May 14, 2024 · 2 comments · Fixed by #363

Comments

@duncanmmacleod
Copy link
Contributor

The _MAX_CONCURRENT_FETCHES module variable in sphinx_immaterial.google_fonts is currently unused:

$ git grep _MAX_CONCURRENT_FETCHES
sphinx_immaterial/google_fonts.py:_MAX_CONCURRENT_FETCHES = 128
<end>

I presume this is supposed to be a way for users to monkeypatch a lower (or higher) concurrency limit to help with network issues, but it isn't used in the call to ThreadPoolExecutor.

Presuming my understanding is correct, is this the right fix?

diff --git a/sphinx_immaterial/google_fonts.py b/sphinx_immaterial/google_fonts.py
index 8f355063..80bef6d3 100644
--- a/sphinx_immaterial/google_fonts.py
+++ b/sphinx_immaterial/google_fonts.py
@@ -64,7 +64,7 @@ def add_google_fonts(app: sphinx.application.Sphinx, fonts: List[str]):
     font_dir = os.path.join(static_dir, "fonts")
     os.makedirs(font_dir, exist_ok=True)

-    with concurrent.futures.ThreadPoolExecutor(max_workers=33) as executor:
+    with concurrent.futures.ThreadPoolExecutor(max_workers=_MAX_CONCURRENT_FETCHES) as executor:

         def to_thread(fn, *args, **kwargs) -> asyncio.Future:
             return asyncio.wrap_future(executor.submit(fn, *args, **kwargs))
@2bndy5
Copy link
Collaborator

2bndy5 commented May 14, 2024

If the purpose of the variable is to allow user customization, then it should be exposed as a config option. I'd rather not encourage monkey patching when it isn't necessary.

And yes, the suggested patch would be the solution if user customization is not preferred for some reason.

2bndy5 added a commit that referenced this issue Jul 5, 2024
2bndy5 added a commit that referenced this issue Jul 5, 2024
resolves #351

use hard-coded default of 128 max_workers

support env var as override
2bndy5 added a commit that referenced this issue Jul 5, 2024
resolves #351

use hard-coded default of 128 max_workers

support env var as override
2bndy5 added a commit that referenced this issue Jul 5, 2024
resolves #351

use hard-coded default of 128 max_workers

support env var as override
@2bndy5 2bndy5 closed this as completed in a930823 Jul 7, 2024
@2bndy5 2bndy5 added the pending release has been resolved but is pending a new release label Jul 7, 2024
@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 21, 2024

The solution for this was released in v0.12.0

@2bndy5 2bndy5 removed the pending release has been resolved but is pending a new release label Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants