Skip to content

MNT: Refactor generate_dir_rst and generate_gallery_rst#1332

Merged
larsoner merged 20 commits intosphinx-gallery:masterfrom
lucyleeow:refactor_gen
Jun 20, 2024
Merged

MNT: Refactor generate_dir_rst and generate_gallery_rst#1332
larsoner merged 20 commits intosphinx-gallery:masterfrom
lucyleeow:refactor_gen

Conversation

@lucyleeow
Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow commented Jun 19, 2024

There has been a few PRs recently adding a fair bit of stuff to generate_dir_rst and generate_gallery_rst, making them quite long and difficult to understand.

Prompted by trying to read the code after #1323.

A lot of simply pulling code out into helper functions, removing redundant things. Notable changes:

  • generate_dir_rst now only writes index.rst file if src_dir is a subsection file
    • previously we over-wrote it later and had a confusing note in generate_gallery_rst about this
    • the old include_toctree already indicated whether it was a subsection or not, we just weren't fully utilising it
  • some re-naming of ambiguous terms like 'has_header', which were all just code for whether the user provided an index.rst file or not
  • Refactor of collect_gallery_files - this is opinionated so I am happy to revert or amend
    • Moves this helper to utils/ - to prevent circular import as I wanted to use it in gen_rst.py. This is technically a public function but a quick search on github seems to suggest only one or two cases of its use, most are forks of SG
    • We now use it twice:
      • get all example files and check for spaces and duplicates (original use)
        • we previously recursed through all sub folder depths. I've amended this to check one level of subfolder as we only make galleries one level deep, so we shouldn't care how the user has named deeper files (?)
        • we only checked .py files previously but with example_extensions it makes sense to check all supported exts
      • get example files to iterate through and generate rst for
        • it makes sense that this is done with the same function as the above check

Copy link
Copy Markdown
Contributor

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Thanks for working on this refactoring, I agree it should help!

@larsoner larsoner merged commit 6953d4e into sphinx-gallery:master Jun 20, 2024
larsoner added a commit to jschueller/sphinx-gallery that referenced this pull request Jun 20, 2024
* upstream/master:
  MNT: Refactor `generate_dir_rst` and `generate_gallery_rst` (sphinx-gallery#1332)
  MNT: Refactor `generate_file_rst` (sphinx-gallery#1335)
  MNT: Refactor `_fill_gallery_conf_defaults` (sphinx-gallery#1334)
  MNT Use `os.sep` everywhere (sphinx-gallery#1333)
  DOC Improve `nested_sections` (sphinx-gallery#1326)
@lucyleeow lucyleeow deleted the refactor_gen branch June 20, 2024 21:56
@QuLogic
Copy link
Copy Markdown
Contributor

QuLogic commented Jul 23, 2024

This appears to have broken (I say broken, as I'm assuming a refactor didn't intend to change behaviour) generating the zip files for the top-level gallery if it has an index.rst; example from Matplotlib build. Specifically, the PR-internal commit 292d3e3 seems to be the root cause. I think this is because the generate_zipfiles call in _finish_index_rst is now in an if sg_root_index? This doesn't generate the zip, but our index.rst is referencing it, causing Sphinx to warn/error out.

(On a side note, large refactors like this are why I dislike squash merging PRs; it makes bisecting more difficult than it should be.)

@lucyleeow
Copy link
Copy Markdown
Contributor Author

Sorry I'll take a look tomorrow.

@QuLogic
Copy link
Copy Markdown
Contributor

QuLogic commented Jul 23, 2024

A change like this does seem to fix it:

diff --git a/sphinx_gallery/gen_gallery.py b/sphinx_gallery/gen_gallery.py
index f8b3c7b..89d8998 100644
--- a/sphinx_gallery/gen_gallery.py
+++ b/sphinx_gallery/gen_gallery.py
@@ -608,13 +608,15 @@ def _finish_index_rst(
         )
         indexst += subsections_toctree
 
-    if sg_root_index:
-        # Download examples
-        if gallery_conf["download_all_examples"]:
-            download_fhindex = generate_zipfiles(
-                gallery_dir_abs_path, app.builder.srcdir, gallery_conf
-            )
+    # Download examples
+    if gallery_conf["download_all_examples"]:
+        download_fhindex = generate_zipfiles(
+            gallery_dir_abs_path, app.builder.srcdir, gallery_conf
+        )
+        if sg_root_index:
             indexst += download_fhindex
+
+    if sg_root_index:
         # Signature
         if app.config.sphinx_gallery_conf["show_signature"]:
             indexst += SPHX_GLR_SIG

Should I open a PR, or do you think this should be refactored somewhere else?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants