Fix `indexst` variable does not exist when own index gallery is first by lucyleeow · Pull Request #1383 · sphinx-gallery/sphinx-gallery

lucyleeow · 2024-10-02T12:16:32Z

Bug introduced by refactor in #1332

When the first gallery in the example gallery loop, in generate_gallery_rst, is one with its own index.rst file, the indexst variable does not exist which is a problem because we pass it to _finish_index_rst. This variable is not used in _finish_index_rst when the gallery has its own index.rst file but _finish_index_rst does various other things so needs to be run always, even when the gallery has its own index.rst.

Adds test.

Will do a patch release once this is merged

for more information, see https://pre-commit.ci

lucyleeow · 2024-10-03T02:30:52Z

A __pycache__ gets added to sphinx_gallery/tests/testconfs/src during the new test_own_index_first test.

When a gallery folder has it's own index.rst file, we do not check for index.rst or gallery header files when we collect subgallery folders. We do not need a index.rst/header file in the sub-gallery folders as we do not write the root gallery index or any nested sub gallery index files when the user provides their own root gallery index.rst.
(Initially I thought we should be more strict and always ensure sub-gallery folders do have an index.rst. or gallery header file, but then realised that technically these are not needed when the user providers their own index.rst file in the root gallery folder)

The test thus fails as the __pycache__ is collected as a subgallery directory, which then obviously fails as it does not have a gallery header file.

@larsoner I've amended the code such that we do not collect any folders that called __pycache__. I think we could further this and avoid collecting any 'dunder' named folder as a nested subgallery as well. WDYT?

larsoner · 2024-10-03T14:06:01Z

I've amended the code such that we do not collect any folders that called pycache. I think we could further this and avoid collecting any 'dunder' named folder as a nested subgallery as well. WDYT?

I think just avoiding __pycache__ is good. It's unlikely but possible that someone in the wild is actually using a dunder folder intentionally somewhere and we'd break things for them. And I'm not sure what folder other than __pycache__ we'd definitely want to exclude. So I say let's keep it as is for now!

larsoner

Just one question about a comment otherwise LGTM!

larsoner · 2024-10-03T14:06:48Z

sphinx_gallery/gen_gallery.py

        subfolders = [
            subfolder
            for subfolder in subfolders
+            # Returns `None` if `index.rst` or gallery header found


Right?

Suggested change

# Returns `None` if `index.rst` or gallery header found

# Returns `None` if neither `index.rst` nor gallery header found

Turns out we are both wrong, this whole thing is pretty confusing.

sphinx-gallery/sphinx_gallery/gen_rst.py

Lines 433 to 458 in 7ddaf92

def _get_gallery_header(dir_, gallery_conf, raise_error=True):

"""Get gallery header from GALLERY_HEADER.[ext] or README.[ext] file.

Returns `None` if user supplied an index.rst or no gallery header file

found and `raise_error=False`.

"""

# First check if user supplies an index.rst and that index.rst is in the

# copyfile regexp:

if re.match(gallery_conf["copyfile_regex"], "index.rst"):

fpth = os.path.join(dir_, "index.rst")

if os.path.isfile(fpth):

return None

# Next look for GALLERY_HEADER.[ext] (and for backward-compatibility README.[ext]

extensions = [".txt"] + sorted(gallery_conf["source_suffix"])

for ext in extensions:

for fname in ("GALLERY_HEADER", "README", "readme"):

fpth = os.path.join(dir_, fname + ext)

if os.path.isfile(fpth):

return fpth

if raise_error:

raise ExtensionError(

"Example directory {} does not have a GALLERY_HEADER file with "

"one of the expected file extensions {}. Please write one to "

"introduce your gallery.".format(dir_, extensions)

)

return None

If there is an index.rst -> return None
If there is a gallery header -> return file path
If there is no index.rst, no gallery header and raise is false -> None

So we want a subsection dir to contain a gallery header, so we check that the return is not None, which only happens when there is a gallery header. Let me amend the comment to be what happens when return is not None, as this is what we are checking for anyway.

Actually the check_for_index parameter name is pretty confusing here, what about we change it to check_for_header or check_for_gallery_header but thats a bit verbose.

Either is fine by me!

changed to check_for_header

lucyleeow · 2024-10-05T05:05:10Z

I think I should release now with just this PR, unless you think we should wait for #1384 or #1385 @larsoner ?

larsoner · 2024-10-07T15:30:29Z

I think those are both close enough we could wait for them!

lucyleeow · 2024-10-08T05:28:16Z

Happy to wait. Should it be a minor release then, or patch still?

larsoner · 2024-10-08T12:54:59Z

Might as well make it a minor release

fix and test

791a21e

lucyleeow added the bug label Oct 2, 2024

lucyleeow and others added 5 commits October 3, 2024 10:45

try?

1932cc2

[pre-commit.ci] auto fixes from pre-commit.com hooks

66b653a

for more information, see https://pre-commit.ci

fix

9b4ecfa

revert

52a02e3

not pycache

68a5fc0

lucyleeow added 3 commits October 3, 2024 12:38

amend doc

5eb6e3e

wording

23b7796

add note

5abd467

larsoner approved these changes Oct 3, 2024

View reviewed changes

lucyleeow added 2 commits October 4, 2024 21:36

fix comment

2ad76e2

rename check for header

d95851a

lucyleeow merged commit a44edd8 into sphinx-gallery:master Oct 5, 2024

lucyleeow deleted the fix_index_only branch October 5, 2024 05:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `indexst` variable does not exist when own index gallery is first#1383

Fix `indexst` variable does not exist when own index gallery is first#1383
lucyleeow merged 11 commits intosphinx-gallery:masterfrom
lucyleeow:fix_index_only

lucyleeow commented Oct 2, 2024 •

edited

Loading

Uh oh!

lucyleeow commented Oct 3, 2024 •

edited

Loading

Uh oh!

larsoner commented Oct 3, 2024

Uh oh!

larsoner left a comment

Uh oh!

larsoner Oct 3, 2024 •

edited

Loading

Uh oh!

lucyleeow Oct 4, 2024

Uh oh!

lucyleeow Oct 4, 2024

Uh oh!

larsoner Oct 4, 2024

Uh oh!

lucyleeow Oct 5, 2024

Uh oh!

lucyleeow commented Oct 5, 2024

Uh oh!

larsoner commented Oct 7, 2024

Uh oh!

lucyleeow commented Oct 8, 2024

Uh oh!

larsoner commented Oct 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	# Returns `None` if `index.rst` or gallery header found
	# Returns `None` if neither `index.rst` nor gallery header found

	def _get_gallery_header(dir_, gallery_conf, raise_error=True):
	"""Get gallery header from GALLERY_HEADER.[ext] or README.[ext] file.

	Returns `None` if user supplied an index.rst or no gallery header file
	found and `raise_error=False`.
	"""
	# First check if user supplies an index.rst and that index.rst is in the
	# copyfile regexp:
	if re.match(gallery_conf["copyfile_regex"], "index.rst"):
	fpth = os.path.join(dir_, "index.rst")
	if os.path.isfile(fpth):
	return None
	# Next look for GALLERY_HEADER.[ext] (and for backward-compatibility README.[ext]
	extensions = [".txt"] + sorted(gallery_conf["source_suffix"])
	for ext in extensions:
	for fname in ("GALLERY_HEADER", "README", "readme"):
	fpth = os.path.join(dir_, fname + ext)
	if os.path.isfile(fpth):
	return fpth
	if raise_error:
	raise ExtensionError(
	"Example directory {} does not have a GALLERY_HEADER file with "
	"one of the expected file extensions {}. Please write one to "
	"introduce your gallery.".format(dir_, extensions)
	)
	return None

Conversation

lucyleeow commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucyleeow commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

larsoner commented Oct 3, 2024

Uh oh!

larsoner left a comment

Choose a reason for hiding this comment

Uh oh!

larsoner Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucyleeow Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

lucyleeow Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

larsoner Oct 4, 2024

Choose a reason for hiding this comment

Uh oh!

lucyleeow Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

lucyleeow commented Oct 5, 2024

Uh oh!

larsoner commented Oct 7, 2024

Uh oh!

lucyleeow commented Oct 8, 2024

Uh oh!

larsoner commented Oct 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lucyleeow commented Oct 2, 2024 •

edited

Loading

lucyleeow commented Oct 3, 2024 •

edited

Loading

larsoner Oct 3, 2024 •

edited

Loading