Skip to content

Improve minigallery directive path input resolution#1360

Merged
larsoner merged 8 commits intosphinx-gallery:masterfrom
lucyleeow:fix_minigallery
Aug 1, 2024
Merged

Improve minigallery directive path input resolution#1360
larsoner merged 8 commits intosphinx-gallery:masterfrom
lucyleeow:fix_minigallery

Conversation

@lucyleeow
Copy link
Copy Markdown
Contributor

closes #1356

Uses relative_to to get target gallery path - in particular improves the part where we figure out if the file is in a subgallery/subfolder. I think the relative_to is more robust and I think it should work as intended.

e.g.,
one glob match: '.../sphinx-gallery/sphinx_gallery/tests/tinybuild/examples_rst_index/examp_subdir2/plot_sub2.py'
example dir path: .../sphinx_gallery/sphinx_gallery/tests/tinybuild/examples_rst_index/

Uses resolve to get abs paths.

cc @story645

Copy link
Copy Markdown
Contributor Author

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

I've described how I think we'd get to the errors below.

I am not sure how best to test it, as this would error on a build. I think I could add a test to test_gen_gallery.py where I copy over a new .rst file that has a minigallery directive that would error (using the sphinx_app_wrapper.srcdir to get the right path).

Would this be the best way to do this @larsoner ? I thought about making a new fixture, like sphinx_app_wrapper but copying different src files, but I think if I can just copy over files using the same sphinx_app_wrapper fixture, it would be better?

)
# `path` is not inside a `examples_dirs`
else:
raise ExtensionError(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the case where the glob match is not inside an examples_dirs (i.e., in old code when len(dirs) = 0).

Comment on lines +163 to +167
elif n_match > 1:
ex_parents, target_dir = max(
[(match[0].parents, match[1]) for match in gal_matches],
key=lambda x: len(x[0]),
)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this may have been what the old len(dirs)>1 was testing before. The only case where I think this could come about is e.g.,

file: /a/b/c/d/file.py
examples dirs (abs path):

  • /a/b/
  • /a/b/c/d (i.e. you have a gallery within a sub-sub-folder of another gallery - this may be done if someone wanted to use more nesting for their galleries)

In this case, I think it makes sense that we just use the longest matching example dir.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, everything in Matplotlib is nested at least 3 deep (eg galleries/gallery_folder/subsection) so that could have been it...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm I don't think that is the same. E.g., examples_dirs in mpl would be e.g., ../galleries/examples, ../galleries/plot_types/. These folders have sub-gallaries/sub-folders, but these subfolderes do not need to be added to examples_dirs as SG auto checks for 1 level of sub-folders.

It's only in the case where you want 2 levels of sub-folders e.g., you also have ../galleries/examples/animation/moreanimation/ in your examples_dirs.

It would be unusual case. And I don't even know if I can be bothered adding a test for this... 🤔

@lucyleeow lucyleeow added the bug label Jul 30, 2024
@larsoner
Copy link
Copy Markdown
Contributor

Would this be the best way to do this @larsoner ? I thought about making a new fixture, like sphinx_app_wrapper but copying different src files, but I think if I can just copy over files using the same sphinx_app_wrapper fixture, it would be better?

Yeah I think reusing the existing sphinx_app_wrapper is reasonable

@lucyleeow
Copy link
Copy Markdown
Contributor Author

lucyleeow commented Aug 1, 2024

This is ready for merge, would be good to get your eye over this @larsoner !

  • added tests for the minigallery cases where the file is not in an examples dir and when its from a file in a gallery nested inside another gallery (here the file path matches 2 examples_dirs)
  • moved tests to use pathlib

@lucyleeow
Copy link
Copy Markdown
Contributor Author

Figuring out sphinx_app_wrapper was a wild ride.

For posterity, it was confusing that the conf_file fixture and the conf_file pytest mark are called the same thing. They are separate and would work fine if they were name differently.

@larsoner
Copy link
Copy Markdown
Contributor

larsoner commented Aug 1, 2024

For posterity, it was confusing that the conf_file fixture and the conf_file pytest mark are called the same thing. They are separate and would work fine if they were name differently.

Feel free to open a PR to rename one of them!

@larsoner larsoner merged commit daeb602 into sphinx-gallery:master Aug 1, 2024
@larsoner
Copy link
Copy Markdown
Contributor

larsoner commented Aug 1, 2024

Thanks @lucyleeow !

@lucyleeow lucyleeow deleted the fix_minigallery branch August 2, 2024 09:24
@lucyleeow
Copy link
Copy Markdown
Contributor Author

Good idea, somehow I am just hesitant about changing existing 'convention'.

clrpackages referenced this pull request in clearlinux-pkgs/pypi-sphinx_gallery Aug 9, 2024
… to version 0.17.1

v0.17.1
-------

**Fixed bugs:**

-  FIX: Fix stability of stored compiled regex `#1369 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1369>`__ (`larsoner <https://github.com/larsoner>`__)
-  ENH: Improve \_sanitize_rst `#1366 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1366>`__ (`timhoffm <https://github.com/timhoffm>`__)
-  Obey prefer_full_module setting when finding backreferences `#1364 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1364>`__ (`QuLogic <https://github.com/QuLogic>`__)
-  Fix linking to class attributes with prefer_full_module `#1363 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1363>`__ (`QuLogic <https://github.com/QuLogic>`__)
-  Improve minigallery directive path input resolution `#1360 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1360>`__ (`lucyleeow <https://github.com/lucyleeow>`__)
-  FIX Allow str path minigallery entries when backreferences off `#1355 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1355>`__ (`lucyleeow <https://github.com/lucyleeow>`__)
-  FIX generate zipfiles when index passed by user `#1353 <https://github.com/sphinx-gallery/sphinx-gallery/pull/1353>`__ (`lucyleeow <https://github.com/lucyleeow>`__)

**Documentation**

(NEWS truncated at 15 lines)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Minigallery directive file checking

3 participants