jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
177 stars 28 forks source link

sphinx_immaterial.google_fonts._MAX_CONCURRENT_FETCHES variable is unused #351

Open duncanmmacleod opened 1 month ago

duncanmmacleod commented 1 month ago

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 commented 1 month ago

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.