-
-
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
PRS: Enable PR previews #2023
PRS: Enable PR previews #2023
Conversation
Built preview: https://peps-aaturner.readthedocs.io/ |
Are there any blockers to moving this forward? |
Blockers to this specifically on non-internal builder support or the general piece of work (including progressing in general & pronouncing judgement on which solution is used)? This PR -- nothing I'm aware of A |
@AA-Turner I think it's a matter of figuring out who makes the call on this and them making the call. Is it the PEP editors, steering council, Documentation WG, people running python.org? I know I don't know who should make the decision as to whether we should move PEP hosting to Read the Docs (last resort is the SC, but no one has gone far enough to throw their arms in the air and say, "I give up, I don't know" 😄). |
Fair enough -- as I said I've been holding off asking -- whenever the decision is made, very happy to be involved in doing whatever's needed in integration or etc otherwise, if wanted. It's not like an extra few weeks, months etc is going to invalidate everything that's currently happened! A |
This is already on the Docs WG tracker: And it sounds like it fits nicely within the WG's charter: However, I'm not sure if the WG is quite up and running yet. @willingc, do you think this is something the WG is ready to make a call on, or if it should in the meantime be deferred to another group such as the PEP editors? |
@hugovk I would let the PEP editors make the call of when to merge. Personally, I think it's a good improvement. |
It looks like I'm 1/7th of "the PEP editors" (https://github.com/orgs/python/teams/pep-editors/members), so I feel qualified to have an opinion. I propose we use this PR to see if there's consensus among the PEP editors; if nobody has an objection, we move forward. I'm not entirely sure though what is being proposed here or what the overall goal is. Is it one of these?
|
@JelleZijlstra Hopefully I can offer some context
yes! (although, there are custom Sphinx adapter scripts still needed, so not quite out-of-the-box sadly)
This wasn't an explicit goal, but in writing the Sphinx I sort of accidentally ended up doing that -- I'd argue that PEPs are somewhat different from documentation, but also consistency can be important. My goal was partially also to make things as simple and fast as possible, so that when deployed hopefully things don't break -- the PEP rendering has up until now been quite low maintenence, and I don't want to cause a lot of people a lot of work in the future for fixing minor breakages etc. (although I'd hope I'd be able to help if able).
I'd say that this is the main open question right now -- more so in terms of which hosting provider is used (GH pages or RTD) -- this PR adds a bunch of logic to enable building the PEPs when not orchestrated by the I believe the intention was always then to hook the hosting provider up to python.org (either as peps.python.org or through a reverse proxy to the current URL). For context, built previews can be found at: Hopefully that provides a quick precis as someone who's been involved, however this summary is by no means authoritative! A |
To a large extent, I don't mind one way or another on each point. With RTD, can it be hosted there while still being served from a URL like "peps.python.org"? That'd make it look properly official. How's RTD with GitHub integration? Can we get a 100% guarantee that, if the build passes on the PR, it'll be able to render the pages? |
GH pages -- yes
I don't think there would be a discrepancy, but as RTD uses a custom dockerfile to run their Sphinx jobs, I guess there is the possibility for such. Currently the GH pages workflow does a full build on each commit to A |
Cool. Then I would be absolutely fine with this change. |
ReadTheDocs also reports a check on PRs, once configured to do so. If you scroll down to the merge "event" for pypa/pip#10279 and click "view details" for the GitHub checks -- you can see that there's a ReadTheDocs check as well -- that's the report from ReadTheDocs on whether the documentation was successfully built. |
And, IMO, one of the main benefits of using ReadTheDocs instead of a GitHub Action + GitHub pages would be that you get PR previews like: https://pip--10279.org.readthedocs.build/en/10279/ (this is the "details" in the GitHub check that ReadTheDocs reports). So, if the build is successful, you can easily see how it renders as well. |
pep_sphinx_extensions/__init__.py
Outdated
|
||
def _update_config_for_builder(app: Sphinx) -> None: | ||
if app.config.html_context["external_builder"]: | ||
app.builder.copysource = False # Prevent unneeded source copying - we link direct to GitHub |
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.
Should this be set to True in the configuration instead being behind a conditional in the extension? Even for internal builds, I don't see why we'd want to show the sources.
https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_copy_source
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.
I can't quite remember why I set this as false in the script instead of in the config -- but it should always be false, this method _update_config_for_builder
does all the stuff build.py
would normally do.
Will look to see if I can move to the config, if so that would be simpler.
Hi @Rosuav / @JelleZijlstra Are you able to prompt the other PEP editors at all please? Only organisation members can view the list Jelle mentioned in his comment (https://github.com/orgs/python/teams/pep-editors/members) so I'm not sure who the other 5 / 7 are, sorry.
Nobody so far has raised an objection, but again I'm not sure that everyone who needs to has seen it -- but I would be keen to support moving this work forwards if possible. Thanks, |
Tagging @python/pep-editors for objections to this. So far, everyone who's posted has been in favour. |
I am one of the PEP editors. I don't feel comfortable reviewing this PR because (like many other tooling PRs for this repo) it's all config stuff that I have no way of assessing. I think @hugovk has a decent grasp of the tooling here, if he approves it I am okay with it. (@Rosuav If you think it's sound that's fine too of course, but you seem to be calling out other PEP editors.) I think the other senior PEP editors are @warsaw and @brettcannon. |
Thanks Guido :) Just to clarify, this PR has been sligtly hijacked for the question of "yea or nay moving PEPs to peps.python.org as distinct from python.org" -- all the actual tooling & config has already been merged and was reviewed by Hugo, Pablo, etc. (Equally sorry if this clarification is uneeeded!) A |
I did not get that, so thanks for the clarification. I think that's actually a Steering Council level decision. It also deserves its own issue (or python-dev or discuss.python.org thread) so the discussion is in one place. |
For what it's worth, I think RTD would be really good: amongst other things we'd get build previews for PRs, making it easy to check things like the RST formatting and so on worked. @AA-Turner Please could you open the discussion on python-dev or https://discuss.python.org > PEPs? |
I'll do so later this evening @hugovk, thanks for the suggestion. What should I be requesting comment on? The merits of moving to peps.python.org overall (as a binary up/down)? A |
As I commented above (#2023 (comment)), I feel like there's a bunch of possible changes in the air. We should be clear about what exactly is proposed to change (the URL where we serve PEPs, the infrastructure used to build PEPs, the infrastructure used to serve PEPs, the styling for the rendered PEPs?). |
So should the "wider audience" message be setting all these out as given and asking for yea/nay, asking for detailed comments, sorting out the process? Sorry for coming back again I'm just wanting to be clear on what the purpose of the wider audience messaging is so that I can write for the right audince & be requesting a specific action from readers (or it could just be a raising awareness item?) A |
In my view it would be best to have a single proposal that answers all the questions I asked above (and maybe more I didn't think of). The proposal can then be modified if there's a consensus in the discussion, but it's still more likely to lead to a productive discussion. It might be just me, but to me statements like "switch to Sphinx" or "switch to RTD" aren't very useful because they don't indicate concretely what will change for people reading or writing PEPs. |
Please see https://discuss.python.org/t/request-for-comment-making-pep-rendering-self-contained/10774 I had to remove all links but it should still make sense. I took the approach of "why should I care" rather than any overly detailed technical aspects, trying to give an overview of the merits of the entire project -- if this isn't what people are after I'm very happy to write a different paper exploring a different angle. A |
@JelleZijlstra the only item I didn't cover from your questions is the look & feel part -- as I think it would be useful to establish if this work is likely to be accepted first, and arguments about colours & typefaces can be had later! Hopefully though I covered your other two questions, although more in-the-round. |
Just checking @AA-Turner ; you're still planning to go ahead with this? |
This adds complexity, but is required for RtD (or similarly set up) build systems to work. Let's wait for the outcome of 676 and what backend is chosen & go ahead with this then if need be. A |
readthedocs.yaml
Outdated
build: | ||
image: testing | ||
|
||
python: | ||
version: 3.9 | ||
install: | ||
- requirements: requirements.txt |
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.
I just wanted to chime in here to say that this config file has been improved a little and you can use "Ubuntu 20.04" Docker image instead of testing
here. See the docs at https://docs.readthedocs.io/en/stable/config-file/v2.html#build
build: | |
image: testing | |
python: | |
version: 3.9 | |
install: | |
- requirements: requirements.txt | |
build: | |
os: ubuntu-20.04 | |
tools: | |
python: "3.9" | |
python: | |
install: | |
- requirements: requirements.txt |
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.
Yeah I saw, this needs a full rebase too. Thanks for pointing it out though!
A
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.
I want this go out of my "Review requested" list. I trust you to land it when the time is right.
af0756f
to
963a4ea
Compare
See #2, python/docs-community#10 for context.
Adds support for building PEPs with Read the Docs (or any external documentation builder).
A