-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Sphinx - docutils #1931
Sphinx - docutils #1931
Conversation
e9b692e
to
bcf5388
Compare
@AA-Turner could you rebase this PR? That way I can approve it to run the CI |
bcf5388
to
2d3f189
Compare
@pablogsal done this - but I think if both CI jobs (core and this one) run at the same time the GH-pages will be overwriten with whichever finishes last |
You need to rebase again on top of your other PR that was just merged :) |
haha give me a sec! thanks for merging, amazing :) |
if TYPE_CHECKING: | ||
from sphinx.application import Sphinx | ||
|
||
# Monkeypatch sphinx.environment.default_settings as Sphinx doesn't allow custom settings or Readers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit brittle, is not a show stopper but I would prefer if we don't do this as this is a potential future breackage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is overriding docutils.conf
, which I didn't want to touch following the discussion about running old + new at the same time, as it is implicitly used in pep2html.py
. I can add a explanatory comment if you'd like, as the end state would be to move this config into docutils.conf
.
This is all just disabling default docutils stuff, apart from the PEP/RFC references, which I believe are on by default but wanted to declare explicitly.
for key, value in node.non_default_attributes().items(): | ||
if key != "classes" or not set(value) <= {"first", "last"}: | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, this can be a if any(...): return False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as in any(key != "classes", not set(value) <= {"first", "last"})
?
Will update, and perhaps break out the subset check to membership tests for clarity
2d3f189
to
c692b36
Compare
@AA-Turner WHat's the best way to test that this works? |
Another thing I noticed is that the links in the rendered docs don't work. For instance, clicking in "PEP 399" in the sidebar shows me the html di of PEP 399, but not the |
#2 (comment) (Éric Araujo) |
Do you know what may be the cause of this? I just ran |
Sorry don't get this one - seems to be working fine on https://aa-turner.github.io/peps/pep-0398/ was this a loally built one? |
Yeah, I built locally. For the record, this is what I get if I click in any link: |
I think that's as you're not running a web server locally? GH pages or nginx etc would serve the |
Yeah, but this is confusing since normally sphinx output doesn't need a webserver. I think this is quite unfortunate. For instance, I can build the python docs and explore my local build without running any webserver. Same for every other project where I have used sphinx. Needing a webserver for local exploration when the pages are static is going to be a big downside. Is this something we can fix? |
No clue on this one, would agree it is most likely theme - more important is if the conent itself is there & has been parsed properly etc. https://aa-turner.github.io/peps/pep-0634/ renders OK - but could we leave theming till #1933 (theming PR)? |
Yeah, that makes sense |
Ahh OK this is a use-case I wasn't aware of, sorry. Two options I think - one is to change the default config and build to e.g. Which would you prefer? |
I am not sure I follow. Normally there is no problem with this. In any sphinx project the gh-pages/rtd/local version don't need changes between them. Unfortunately I don't get why this configuration is doing this folder setup that produces this problem. I would prefer to not have different configurations for CI/local/server and I would prefer that the links work without a webserver |
For the current /dev/peps hosting each PEP page is in its own URL (https://www.python.org/dev/peps/pep-3131/) - the easiest way to replicate this without using arcane reverse proxy logic on the build side was to build to directories. The alternative is to move to having e.g. Simpler on docs.python as the url endpoints are e.g. |
Might be overthinking the whole thing but that is a precis of my logic/thinking, hopefully makes some amount of sense :) |
Ok, I think I understand what are we trying to achieve. That is quite unfortunate but is ok, let's move on |
Adding a config option either way would be pretty easy as a temp fix, and can be done at any time so let me know if its wanted. |
One questio, why I cannot find PEP0 in https://aa-turner.github.io/peps/contents/# ? |
That may be a good compromise, actually. Could we do that on a separate PR? Add me as a reviewer please (or CC me in a comment) |
As that's PR #1932! |
Gotcha! |
Sounds good |
Ok, let's move to #1932 |
See #2, #1385 for context. Superseeds #1566.
This is the docutils parsing, transforms and writing part, building on PR #1930. It contains a pseudo-package,
sphinx_pep_extensions
, which itself contains:Docutils parsing:
PEPParser
- collates transforms and interfaces with Sphinx corePEPRole
- deals with :PEP:blah
in RST sourceDocutils transforms:
PEPContents
(Creates table of contents without page title)PEPFooter
(Dels with footnotes, link to source, last modified commit)PEPHeaders
(Parses RFC2822 headers)PEPTitle
- Creates document title from PEP headersPEPZero
- Masks email addresses and creates links to PEP numbers from tables inpep-0000.rst
Docutils HTML output:
PEPTranslator
- Overrides to the default HTML translator to enable better matching of the current PEP styles