8000 FIX Allow str path minigallery entries when backreferences off by lucyleeow · Pull Request #1355 · sphinx-gallery/sphinx-gallery · GitHub
[go: up one dir, main page]
Skip to content

FIX Allow str path minigallery entries when backreferences off #1355

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 30, 2024
Merged

FIX Allow str path minigallery entries when backreferences off #1355

merged 4 commits into from
Jul 30, 2024

Conversation

lucyleeow
Copy link
Contributor

Closes #1354

Amend the minigallery directive, such that when backreferences_dir is None, we simply treat all entries as str paths and glob them.

I thought about adding additional check to the minigallery entries, to make sure they are not objects. But it would be very unusual to have a file named e.g., sphinx_gallery.sorting.ExplicitOrder in the source dir?
Happy to add a check though, WDYT @larsoner ?

Also fixes a bug where we were not converting examples_dirs and gallery_dirs to list when given as str.

Note to self: look into: #1226 (comment)

@lucyleeow lucyleeow added the bug label Jul 25, 2024
lucyleeow
lucyleeow commented Jul 25, 2024
View reviewed changes
@lucyleeow
Copy link
Contributor Author

@larsoner will release after this FYI

story645
story645 reviewed Jul 25, 2024
View reviewed changes
@story645
Copy link
Contributor

GitHub ate my review comment, but my guess is I wrote that error message when I was trying to debug relative paths, so I'd put in "../../examples" as input and the example dir would be "/examples" and I think wording could be "no matched found for {dir}, files must be in {example_dir}".

@lucyleeow
Copy link
Contributor Author

"../../examples" as input

As input to the minigallery directive? But would glob find anything if it was the wrong relative path?

@story645
Copy link
Contributor

"../../examples" as input

As input to the minigallery directive? But would glob find anything if it was the wrong relative path?

This error gets triggered when it doesn't find anything(the condition is !=1)

@lucyleeow
Copy link
Contributor Author
lucyleeow commented Jul 25, 2024 •
edited
Loading

Sorry I mean if the input is wrong the glob above would not find anything:

sphinx-gallery/sphinx_gallery/directives.py

Line 105 in 2fc5067

elif paths := Path(src_dir).glob(obj):

I guess it could find a file, but not inside the examples_dir. In this case dirs == 0 and the error message should probably say that no matching files found inside examples_dirs.

I don't know in what case dir > 1 though? One input obj str would need to contain more than one examples dirs name? How would this come about?

Edit: I guess you could have 2 examples_dirs, where one was a sub-string of another? But is this a problem if the glob results in different paths?

@story645
Copy link
Contributor
story645 commented Jul 25, 2024 •
edited
Loading

or I guess it could find the same file, but not inside the examples_dir

fun bit about using relative paths - same file inside the examples_dir folder, but the paths don't evaluate to the same path , but agree that "no matches found for {dir}, files must be in {example_dir}" wording would probably be better.

But is this a problem if the glob results in different paths?

So I think this is cause obj.find(e) can be stupidly brittle but I couldn't think of a better way to do this bit - I think something like input needs to be relative path but making the thumbnail needed this path - an improvement/follow up if I have time would be to make a list of (example/thumbnail/intro/title) so examples don't need to get reparsed here and that would probably clean up some of this.

@lucyleeow
Copy link
Contributor Author

same file inside the examples_dir folder, but the paths don't evaluate to the same path ,

sorry I do not follow what you mean. Can you give an example?

I think something like input needs to be relative path but making the thumbnail needed this path

Are you are talking about:

sphinx-gallery/sphinx_gallery/directives.py

Line 151 in 2fc5067

target_dir = target_dir / path.relative_to(example_dir).parent

Could you give an example? E.g., if I wanted to test this error, what would the test look like?

@story645
Copy link
Contributor
story645 commented Jul 25, 2024 •
edited
Loading

Could you give an example? E.g., if I wanted to test this error, what would the test look like?

this will trigger the error: ../../tinybuild/examples/*/*.py and back on the original PR I went in circles trying to find a way to check that they resolve to the same location, so maybe the error message should be something like:

 f"Error in minigallery file lookup: input path {obj} matches to {dirs}. Input must be start with one of the following paths: {examples_dirs}"

@story645
Copy link
Contributor

Also apparently originally this was written specifically for the >1 case (#1226 (comment)) which I can't seem to trigger so 🤷‍♀️ what it's supposed to communicate is I think something like "incorrect or ambiguous input, can't resolve to matching examples folder"

@story645 story645 mentioned this pull request Jul 25, 2024
Merged
5 tasks
@lucyleeow
< 8000 span class="Button-content"> Copy link
Contributor Author
lucyleeow commented Jul 26, 2024 •
edited
Loading

this will trigger the error: ../../tinybuild/examples//.py

I get this error:

sphinx.errors.ExtensionError: Error in minigallery file lookup: input=../../tinybuild/examples//.py, matches=[], examples_dirs=['../examples/', '../examples_with_rst/', '../examples_rst_index', '../examples_README_header']

But again we are in the case where there are NO matches - i.e., glob found files, but the files aren't in examples_dirs.

What is an example of a case where there are >1 matches?

Edit: just saw:

which I can't seem to trigger so 🤷‍♀️

lucyleeow
lucyleeow commented Jul 26, 2024
View reviewed changes
sphinx_gallery/directives.py
@@ -67,12 +70,16 @@ def run(self):
# Retrieve the backreferences directory
config = self.state.document.settings.env.config
backreferences_dir = config.sphinx_gallery_conf["backreferences_dir"]
if backreferences_dir is None:
logger.warning(
Copy link
Contributor Author
@lucyleeow lucyleeow Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also possibly silly question, but I note a self.warning (using Directive.warning) used above - in what case should we be using the Directive warning and in what case should we be using our logger?

I tried self.warning and it did not output during build. Where does the warning output to when self.warning is used ..?

@lucyleeow
Copy link
Contributor Author
lucyleeow commented Jul 26, 2024 •
edited
Loading

Okay I think I'm going to open another issue for the match error, because it probably shouldn't hold up this PR.

This is ready for review/merge. I just had a question about warnings: #1355 (comment) (thank you 🙏 )

@story645
Copy link
Contributor

What is an example of a case where there are >1 matches?

Honestly can't come up w/ one, so my guess is that a refactor of code before this point maybe made this case unreachable but not sure. 🙃

@lucyleeow lucyleeow mentioned this pull request Jul 26, 2024
Closed
@lucyleeow
Copy link
Contributor Author

Let's continue the conversation in #1356

@story645 any insights on #1355 (comment) ? I see you use a self.warning in the directive?

@story645
Copy link
Contributor
story645 commented Jul 26, 2024 •
edited
Loading

Let's continue the conversation in #1356

? I see you use a self.warning in the directive?

Pretty sure that code was in the directive before I touched it, I just added the ExtensionError, sorry :/ either that or I copy/pasted it from @melissawm's implementation of a different directive.

@lucyleeow
Copy link
Contributor Author

an improvement/follow up if I have time would be to make a list of (example/thumbnail/intro/title) so examples don't need to get reparsed here and that would probably clean up some of this.

re-reading, could you expand? Where would the list live?

@story645
Copy link
Contributor

re-reading, could you expand? Where would be the list live?

I was thinking json file or the like, and not sure where - figure that would be part of PR discussion.

@lucyleeow
Copy link
Contributor Author

Ah okay, maybe with the thumbnail images as image metadata. But we'd have to consider whether it's worth all the extra work generating the files, performance wise (especially as some projects may not use the minigallery directive). Anyway, things to consider.

@larsoner I'll merge this next week, in case you want to take a look.

8000

@story645
Copy link
Contributor

especially as some projects may not use the minigallery directive

Yeah, I figure I can open an issue w/ a more thought out proposal if there's soft support & this can be configurable (cache_gallery_entries)

@lucyleeow
Copy link
Contributor Author

Merging this.

I think there is no need to add extra checks to prevent us from glob-ing object inputs:

adding additional check to the minigallery entries, to make sure they are not objects. But it would be very unusual to have a file named e.g., sphinx_gallery.sorting.ExplicitOrder in the source dir?

But @larsoner let me know if you disagree!

@lucyleeow lucyleeow merged commit b47628c into sphinx-gallery:master Jul 30, 2024
18 checks passed
@lucyleeow lucyleeow deleted the backref branch July 30, 2024 03:11
@lucyleeow lucyleeow mentioned this pull request Jul 30, 2024
Open
clrpackages referenced this pull request in clearlinux-pkgs/pypi-sphinx_gallery Aug 9, 2024
@fenrus75
pypi-sphinx_gallery: Autospec creation for update from version 0.17.0…
90cfa79 
… 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
Reviewers

@story645 story645 story645 left review comments

@larsoner larsoner larsoner left review comments

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

minigallery raises cryptic error when backreferences_dir is None
3 participants
@lucyleeow @story645 @larsoner
0