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

The PEP Rendering System is dead; long live the PEP Rendering System #2399

Merged
merged 2 commits into from
Mar 10, 2022

Conversation

AA-Turner
Copy link
Member

The redirects are now active (python/pythondotorg#2006).

A solemn farewell to the old rendering system.

A

closes #2

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you! A few quick comments.

.github/CODEOWNERS Show resolved Hide resolved
Makefile Show resolved Hide resolved
generate_rss.py Show resolved Hide resolved
pep-0676.rst Show resolved Hide resolved
@hugovk
Copy link
Member

hugovk commented Mar 9, 2022

The redirects are now active (python/pythondotorg#2006).

It's really nice to see https://peps.python.org/pep-0008/ already active!

(Probably waaay too late too suggest this, so please ignore if so! Now I see the domain used for real, https://peps.python.org/0008/ could be a nice alternative.)

It's nearly two years since your first PR #1385 toward this, thank you Adam for all your hard work, patience and perseverance!

@Rosuav
Copy link
Contributor

Rosuav commented Mar 9, 2022

Awesome to see.

@hugovk
Copy link
Member

hugovk commented Mar 9, 2022

There's a bunch of Sphinx warnings:

https://github.com/python/peps/runs/5486613968?check_suite_focus=true#step:5:47

But I suggest dealing with those in followups and then turning warnings into errors (-W --keep-going).

@AA-Turner AA-Turner requested a review from hugovk March 9, 2022 22:43
@CAM-Gerlach CAM-Gerlach linked an issue Mar 10, 2022 that may be closed by this pull request
@CAM-Gerlach
Copy link
Member

This also fixes #2270 , BTW, since the problem is no longer present in the new build system (as verified there).

There's a bunch of Sphinx warnings:

Yep, I've had fixing those on my to-do list so we could use the best-practice python -bb -X dev -W error -m sphinx -W -n --keep-going invocation to catch and surface warnings at either the Python or Sphinx levels in our CI, rather than having them buried in the build logs. It just hasn't been the biggest priority relative to other more user-impactful improvements.

@hugovk hugovk merged commit 4bdabc6 into python:main Mar 10, 2022
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.

One breaking regression and one textual suggestion. Otherwise, LGTM.


.. code-block:: bash
# or, if you don't have 'make':
python3 build.py
Copy link
Member

@CAM-Gerlach CAM-Gerlach Mar 10, 2022

Choose a reason for hiding this comment

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

Suggested change
python3 build.py
python build.py

There is no python3 command on Windows, i.e. the major platform without make. This is why the original used python, which also works on any platform provided an environment is activated (which the original summary explicitly mentioned, but was removed here).

Comment on lines +48 to +49
PEP draughting aids
-------------------
Copy link
Member

Choose a reason for hiding this comment

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

I'd never heard the word "draughting" before (my first impression was that it had something to do with heavy alcohol consumption), and had to look it up; it seems to be a BrE-specific term and at least as a non-BrE speaker/writer, it was distracting, confusing and looked out of place here. I'd suggest avoiding dialect-specific terminology, especially in such a prominent place in our README, and instead using more nuetral, internationally-understood wording.

For example, also being more consistent with the phrasing of the previous headings,

Suggested change
PEP draughting aids
-------------------
Linting PEPs
------------

@astrojuanlu
Copy link

Congrats for this milestone! 🎉

It is my understanding that this PR implements a self-hosted rendering for the PEPs on GitHub Actions + GitHub Pages. The other option that was being discussed was hosting on Read the Docs. This decision was intentionally left out of the PEP for good reasons, but I lost track of where the final call was made - I've been following this discussion very closely in several places (GitHub issues here and there, Discourse, etc) and I don't recall reading anything, so I'm probably missing something. Could someone share some pointers on the rationale? Just seeking to understand the decision-making process here (and how it relates to, say, the CPython Docs Workgroup).

(Ironically, the GitHub project is called "Host PEPs on Read the Docs" https://github.com/python/peps/projects/1)

Disclaimer: I used to work on RTD when the PEP was proposed and I weighed in several times, even though I'm no longer employed there.

@hugovk
Copy link
Member

hugovk commented Mar 10, 2022

There's a PR for RTD at #2023, it was pending a decision on the PEP (accepted 🎉) and what backend is chosen.

As far as I'm aware, a decision wasn't made on the backend as such. I'm also for RTD because build previews for PRs is so useful.

Let's open a new issue here or thread at https://discuss.python.org?

(Or could continue in https://discuss.python.org/t/request-for-comment-making-pep-rendering-self-contained/10774, but that's the PEP RFC, so it's probably served its purpose and better starting fresh?)

See also python/docs-community#10.

@CAM-Gerlach
Copy link
Member

While I'm attracted to the appeal and simplicity of self-hosting (and is what I maintain elsewhere), infra commonality with other Python projects and the simplicity of supporting build previews vs. e,g, setting up Netlify or viewing a downloaded artifact (a huge value-add, IMO) seems to favor to RTD.

As a side note, if we do host PEPs on RTDs, since it is a separate platform from GH we should ensure we're not susceptible to "bus factor", etc,, such that at least three-ish people have the appropriate permissions there (perhaps the three PEP editor team maintainers and/or Adam). It seems like that's been a bit of a lately bottleneck over on Docs-Community for doing something very similar.

@AA-Turner
Copy link
Member Author

I'll set up previews through read the docs, although I want to keep the core rendering on GH pages for now (I'm going through clean up tasks for 676 but will be away from this repo for a few weeks after that. Will re-visit the overall question in ~early May/late April.

A

CAM-Gerlach added a commit to CAM-Gerlach/peps that referenced this pull request Mar 19, 2022
CAM-Gerlach added a commit that referenced this pull request Mar 24, 2022
#2447)

* PEP 1, 12: Update outdated references to Docutils to point to Sphinx

* PEP 1, 12: Update and simplify outdated legacy rendering workflow steps

* Readme: Fix minor issues introduced in PR #2399

* Infra: Update contents to include PEP rendering docs and avoid warnings

* Lint: Update hardcoded PEP link checker to detect new PEP URLs
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.

PEP 360 table looks broken Build PEPs using Sphinx
7 participants