Skip to content
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.

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

PRS: Explicitly list files to render #2415

Merged
merged 2 commits into from
Mar 14, 2022
Merged

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Mar 11, 2022

Closes #2405, closes #2404

This PR specifies a whitelist of files to include in the rendering. It currently monkeypatches Sphinx, although I hope to add support upstream for this in general (specifiable via conf.py etc)

A

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

I'm not sure why its necessary to do a hacky monkeypatch of Sphinx with a bunch of bespoke code, when we can achieve the same result with a handful of static exclude patterns, as in #2405 (and then hopefully transition to an even simpler solution if and when Sphinx upstreams support).

However, I could see the benefit of this approach, though, if this would read the include patterns from the config as also intended for upstream Sphinx, such that the transition to the non-monkeypatched version would be seamless and require only deleting the monkeypatch, while avoiding hardcoding the patterns in the code.

There's another issue, at least as currently implemented—it looks like this prevents Sphinx from rendering .rst files even in pep-NNNN subdirectories, e.g. PEP supplementary/reference materials, appendices or other auxiliary documents not part of the main PEP. This was a main benefit of PEP 676, at least for me, and something I'm actively relying on with PEP 639 to reduce its length by two thirds without throwing away the useful reference materials, particularly the original author's work. At the very least, it should render .rst files found in pep-NNNN directories, unless explictly excluded (pep-0012).

@AA-Turner
Copy link
Member Author

this prevents Sphinx from rendering .rst files even in pep-NNNN subdirectories, e.g. PEP supplementary/reference materials, appendices or other auxiliary documents not part of the main PEP

No PEP has ever needed this to date, so I'd argue making explict those that require subdirectory organisation would be workable.

I'll refactor to read from conf.py.

A

@AA-Turner
Copy link
Member Author

Refactored to keep configuration in conf.py (this is a nicer solution, thank you for prompting me to look into it).

A

Copy link
Member

@CAM-Gerlach CAM-Gerlach 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 the changes. I have one request to make the matching more precise, follow PEP 1 and PEP 12 and avoid false positives; aside from that, I'm okay with this approach superseding #2405 on the condition that we work to get this actively upstreamed into Sphinx so we don't have to maintain this monkeypatch ourselves indefinitely.

I meant to suggest this before, but it looks like GitHub mysteriously dropped my previous review suggestion (and in any case, it was to the previous hardcoded constant, since you hadn't yet implemented the conf.py support).

conf.py Outdated
# Required for Sphinx
"contents.rst",
# PEP files
"pep-????.???",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"pep-????.???",
"pep-[0-9][0-9][0-9][0-9].rst",
"pep-[0-9][0-9][0-9][0-9].txt",

More precise; right now it will pick up anything starting with pep- and four characters, with any standard three-character extension. E.g. pep-0001.bak, pep-data.pkl, pep-0012.tmp, pep-help.rst, etc. This just detects PEPs that match the format in PEP 1/ PEP 12.

# PEP files
"pep-????.???",
# PEP ancillary files
"pep-????/*.rst",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"pep-????/*.rst",
"pep-[0-9][0-9][0-9][0-9]/*.rst",

More precise like the above; right now any rst files in a directory named pep- and four characters will be picked up, e.g. pep-help/. pep-temp/, etc. This only detects directories that match the format in PEP 1/PEP 12.

@AA-Turner
Copy link
Member Author

I made the change to specific suffixes.

A

@JelleZijlstra
Copy link
Member

What are the prospects of getting this into upstream Sphinx? I'd rather not monkeypatch it.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Mar 13, 2022

Also, what's the downside of properly specifying that the PEP files and directories must have numeric suffixes, as required by PEP 1 and PEP 12? Otherwise, it could match .txt and .rst files with any arbitrary four-character suffix, such as pep-info.txt, and directories with any arbitrary four characters following pep-, such as pep-help/. This is the glob that @hugovk and I agreed on for the RSS generator in #2407 , albeit that commit was lost when it was incorporated it into #2413 instead. Sure, it may not currently match any files, but what's the harm in being safe here (given it required extra work to manually implement a different change rather than just clicking apply on the suggestion)?

@AA-Turner
Copy link
Member Author

What are the prospects of getting this into upstream Sphinx?

Hopefully pretty good, the Sphinx maintainers are quite responsive. I won't have time to propose it upstream until April though, hence the fix here.

A

@AA-Turner
Copy link
Member Author

I'm merging this now as its been tested as working (by at least myself and Nick), and long term I think it is a better solution.

If Sphinx rejects the change, I'll make sure to revert this and implement #2405.

A

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.

make sphinx Breaks when Certain Untracked Files are Present
4 participants