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

Add sphinx support #1385

Closed
wants to merge 110 commits into from
Closed

Conversation

AA-Turner
Copy link
Member

[potentially resolves #2]

This adds sphinx as a build process, potentially resolving issue #2. Note that there are a couple of hacks - on generating the title and contents elements for each PEP's HTML representation - I (personally) think that it would be clearer to have these directives in the source PEP files but I'm not sure if this would be allowed. (If it is I am of course happy to do this work!)

I haven't touched the bash.deploy script, and I endeavoured to ensure that the makefile still outputs a tarball of the files and the RSS file, retaining what was previously happening.

Sphinx generates two index files (contents and genindex) which I think are uncessary given PEP 0, but I think that if the relbars at the top and bottom are disabled then there will be minimal impact (I looked for a way to disable generation but couldn't find one).

Finally a few misc notes:

  • The theme is currently the default classic theme - I imagine that this will want changing but kept this just for ease at the moment
  • I wasn't sure what to put for copyright/author/release but I'm not sure they're needed given the right theme
  • I had to redefine PEPHeader and PEPContents from docutils.transforms.peps - I don't know if they should be updated there and then simply imported, or who the right person to ask on that would be, but happy to do the work once a answer is arrived at!

Thanks,
Adam

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@AA-Turner

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@AA-Turner
Copy link
Member Author

Signed agreement

@AA-Turner
Copy link
Member Author

Also of note is that I wasn't sure what minimum version of Python I had to support so I went off of travis, which targets 3.7, and used newer features like f-strings etc. If there is a more formal minimum please let me know and I'll make the needed changes.

@merwok
Copy link
Member

merwok commented Apr 27, 2020

It’s nice to see your progress here!

The CLA check is based on bugs.python.org accounts: do you have one there, with your github username in your profile? Please check the steps detailed in the link in the bot’s message #1385 (comment)

The PEP pages repeat the PEP tile in the table of contents, which adds a level of nesting that’s not wanted. Could you address this?

@AA-Turner
Copy link
Member Author

It’s nice to see your progress here!

Thanks!

The CLA check is based on bugs.python.org accounts: do you have one there, with your github username in your profile? Please check the steps detailed in the link in the bot’s message #1385 (comment)

I signed 2 days ago - my latest push seems to have triggered the Kight to say Ni!

The PEP pages repeat the PEP tile in the table of contents, which adds a level of nesting that’s not wanted. Could you address this?

Will do

.travis.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I cannot review this PR, it's too big. Try maybe to split it into smaller parts, or try to only build a subset of PEPs.

docutils.conf Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member Author

I cannot review this PR, it's too big. Try maybe to split it into smaller parts, or try to only build a subset of PEPs.

Hi Victor -- thanks for the comment! I don't have much experience with breaking up PRs, especially as everything is interdependent but will have a go. I think the main themes / sub-areas I'd identify would be:

  • sphinx core support,
  • current pydotorg support,
  • custom docutils parsers,
  • PEP0 process changes,
  • theming.

If you have any suggestions on natural breakpoints they would be appreciated, otherwise I will have a go shortly.

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@JimJJewett

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

- RFC2822 headers work
- Contents page generated
- Title autogeneration not working
- PEP 0 not started
Also add pep_parser as I missed it
@AA-Turner
Copy link
Member Author

I cannot review this PR, it's too big. Try maybe to split it into smaller parts, or try to only build a subset of PEPs.

Hi Victor (@vstinner) - I have now broken the PR into smaller chunks, apologies for the delay. Please let me know if smaller logical units would be useful and I will do so.

#1565 - core PR
#1566 - docutils core, transforms/parsing/writing
#1567 - PEP 0 generation & hooks
#1568 - theming (both CSS and templating)
#1569 - pythondotorg compatability - very much a temporary layer.

Thanks,
Adam

@hugovk
Copy link
Member

hugovk commented Sep 6, 2020

Thanks for splitting them into logical blocks, but at least each one can't be merged independently as the CI fails, because they're still dependent on each other, so that may still be a review blocker.

Is it all possible to split out individual chunks that could be merged independently, and so the CI passes? They might be small, but would help reduce the size a bit.

I don't have merge rights, but perhaps a good approach would be to aim to have parallel deploys: the old, existing one, and also a new one to Read the Docs. Then once RTD has been verified, it could be set as the main location and the old one disabled.

It would be really great to move forward with this, especially as it's difficult to build python.org locally for testing things like #1577.

@AA-Turner
Copy link
Member Author

AA-Turner commented Sep 6, 2020

Hi Hugo,

The CI fails partially as all the PRs are using the 'final' state travis recipe - if I reverted to the current (and added the pep2html script back) then it'd pass. CI passing or not isn't really saying much as it isn't verifying the state of the PR currently (If CI passes are required to merge then I could revert .travis.yml etc of course).

To be honest I'm not sure what having parallel deploys would acheieve, or what that would look like. I could add the current build process back (pep2html etc) - that might enable merging with the Sphinx process 'dark' until switchover?

Also just for clarity - this currently builds to GitHub Pages not readthedocs, for flexibility. RE #1577, pygments is currently supported in this PR!

Sorry for the request for clarity but I don't want to do a lot more work for the PR to continue to stall - I'm also really keen to move forwards on this. However if doing something would help reduce burden on reviewers then I'm really keen to understand that and get it done!

Thanks,
Adam

@hugovk
Copy link
Member

hugovk commented Sep 6, 2020

Parallel deploys may allow a quicker merge: the new deploy on GitHub Pages wouldn't yet be a live version, and further tweaks (in later, smaller PRs) can be made before committing to turning off the old deploy.

But as I say, I don't have merge rights here, and completely understand you don't want to do a lot more work for nothing, so we're probably best off waiting for guidance from a maintainer.

@AA-Turner
Copy link
Member Author

That makes sense - I think that would be a useful thing to do, but will wait for guidance before going ahead

@AA-Turner
Copy link
Member Author

@merwok @hugovk @vstinner Do you know who would be best to ask / what would be best to do re getting a steer/guidance from someone (xref #1385 (comment))?

I admit I've let this one sit a bit, but with a plan of action to enable this to be reviewed hopefully a final push could get this to a more mergeable state? Hopefully would prove a quality of life improvement!

I initially broke this up into PRs 1565-9, but appreciate Hugo and other's comments that this may not have been the best way to approach it to enable reviewers - again advice here would be appreciated for a general plan of attack?

Thanks,
Adam

@hugovk
Copy link
Member

hugovk commented Apr 15, 2021

Hi! Same thoughts as above: each PR needs to be a fully working unit, ideally as small as possible for ease of review, and must leave things working afterwords.

Parallel deploys to each system would allow the old system still to be the "master" one, and would also allow the new system to be demoed, and incrementally updated.

And I don't have merge rights here so these are just suggestions :)

@AA-Turner
Copy link
Member Author

AA-Turner commented Apr 15, 2021

Thanks Hugo!

Parallel deploys to each system would allow the old system still to be the "master" one, and would also allow the new system to be demoed, and incrementally updated.

Makes sense, will roll-back some of the more aggressive stuff with wholesale removing all the 'old' code!!

Hi! Same thoughts as above: each PR needs to be a fully working unit, ideally as small as possible for ease of review, and must leave things working afterwords.

If I structure the PRs as pulls to a central PR (which itself pulls to master), would that help towards this balance? There's a core subset of changes -- I can try and factor out as much as possible, I guess trying to find that balance between independent/atomic feature sets and not duplicating review effort by having the same core stuff accross multiple PRs?

Adam

@hugovk
Copy link
Member

hugovk commented Apr 15, 2021

If I structure the PRs as pulls to a central PR (which itself pulls to master), would that help towards this balance? There's a core subset of changes -- I can try and factor out as much as possible, I guess trying to find that balance between independent/atomic feature sets and not duplicating review effort by having the same core stuff accross multiple PRs?

Best answered by a maintainer. Either way could work, personally I'd prefer to get things into master sooner as that's what counts.

@AA-Turner
Copy link
Member Author

Redid PRs, including a fresh commit history so hopefully easier to review. Significantly reduced the size of the master PR - including splitting out the RSS logic from it.

Tried a few different ways of splitting up the theme/docutils PRs, but I wasn't able to find a satisfying way as they pretty much self-contained logical units.

Spent quite a bit of time also going over and adding explanatory comments and type hints etc to aid understanding for future readers - especially with the docutils stuff hard to understand what's going on in places!

Please do let me know if/what extra I can be doing to help the review effort.

Thanks,
Adam

@hugovk
Copy link
Member

hugovk commented Apr 21, 2021

So should maintainers review (and merge) #1930 first?

And then can independently review (and merge) #1931 - #1934?

@AA-Turner
Copy link
Member Author

That might make sense, I'd then be able to rebase 1931 -- 1934 on master, and reduce the changeset sizes for the later reviews.

A

pablogsal pushed a commit that referenced this pull request Jun 8, 2021
See #2, #1385 for context. Superseeds #1565.

This is the minimal core Sphinx support part, adding a bare minimum of useful things to get Sphinx to build and deploy, whilst not affecting the current build system. There is no theming or custom parsing needed to properly deal with PEPs.

- `build.py` - build script
- `conf.py` - Sphinx configuration
- `Makefile` - new targets for Sphinx
- `.gitignore` - add ignores for `venv` and `package` directories
- `contents.rst` - Sphinx page to discover all PEPs
- `deploy-gh-pages.yaml` - builds and deploys to github pages
- `requirements.txt`
pablogsal pushed a commit that referenced this pull request Jun 9, 2021
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 core
- `PEPRole` - deals with :PEP:`blah` in RST source

### Docutils 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 headers
- `PEPZero` - Masks email addresses and creates links to PEP numbers from tables in `pep-0000.rst`

### Docutils HTML output:
- `PEPTranslator` - Overrides to the default HTML translator to enable better matching of the current PEP styles
pablogsal pushed a commit that referenced this pull request Jun 15, 2021
See #2, #1385 for context.

This is the RSS generation part, building on PR #1930. It contains the logic for generating RSS

This was originally in #1385 and #1565, split out for ease of review
pablogsal pushed a commit that referenced this pull request Jun 30, 2021
See #2, #1385 for context. Superseeds #1568.

This is the Sphinx-theming part, building on PR #1930.

### Stylesheets:
- `style.css` - styles
- `mq.css` - media queries

### Jinja2 Templates:
- `page.html` - overarching template

### Javascript:
- `doctools.js` - fixes footnote brackets

### Theme miscellany
- `theme.conf` - sets pygments styles, theme internals
@AA-Turner
Copy link
Member Author

Closing this one as all of the individual sub PRs have now been merged in. Thanks for your support at various times @merwok // @hugovk -- your reviews (especially back in 2020!) were really useful!

@AA-Turner AA-Turner closed this Jun 30, 2021
@hugovk
Copy link
Member

hugovk commented Jul 1, 2021

And thank you @AA-Turner for your good work and patience!

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.

Build PEPs using Sphinx
5 participants