Repository navigation
Link to the correct PEP source file. - #1195
Conversation
Some PEPs are using the .rst extension, and many are in .txt. The source link currently assumes that all the PEPs are using the .txt extension. Fixes python/peps#431
berkerpeksag
left a comment
berkerpeksag
left a comment
There was a problem hiding this comment.
Looks good to me, I just left some comments on cosmetic issues. Thanks!
|
|
||
| source_link = "https://github.com/python/peps/blob/master/pep-{0}.txt".format(pep_number) | ||
| pep_ext = ".txt" | ||
| pep_rst_source = os.path.join(settings.PEP_REPO_PATH, |
There was a problem hiding this comment.
That's pretty clever! I never thought about using settings.PEP_REPO_PATH.
| data['content'] = str(pep_content) | ||
|
|
||
| source_link = "https://github.com/python/peps/blob/master/pep-{0}.txt".format(pep_number) | ||
| pep_ext = ".txt" |
There was a problem hiding this comment.
Style nit: I'm trying to use single quotes in new code. Can we change these to single quotes?
| if os.path.exists(pep_rst_source): | ||
| pep_ext = ".rst" | ||
|
|
||
| source_link = "https://github.com/python/peps/blob/master/pep-{0}{1}".format( |
| ) | ||
|
|
||
| @override_settings(PEP_REPO_PATH=FAKE_PEP_REPO) | ||
| def test_source_link_rst(self): |
There was a problem hiding this comment.
Is it possible to make the content of the PEP shorter?
|
Thanks @berkerpeksag! I made the requested changes. Please re-check. |
Naereen
left a comment
Naereen
left a comment
There was a problem hiding this comment.
LGTM, but I suggest a more generic implementation (and not specific to the pair .txt/.rst), if possible and if someone else agrees.
| pep_rst_source = os.path.join(settings.PEP_REPO_PATH, | ||
| 'pep-{}.rst'.format(pep_number)) | ||
| if os.path.exists(pep_rst_source): | ||
| pep_ext = '.rst' |
There was a problem hiding this comment.
That's an OK solution for just .txt or .rst.
Wouldn't it be possible to write a generic test, by looking for a glob pattern pep-{}.* and selecting the extension from that?
Because if the documentation system goes to .md or whatever new extension, the website code would again have to be changed here!
There was a problem hiding this comment.
I'm pretty sure we aren't going to support another documentation format (especially Markdown) within the Python organization. Plus, we are going to move all PEP infrastructure to RtD so YAGNI :)
There was a problem hiding this comment.
And if we ever did, we'd probably move generation of the source link inside pep2html.py in the PEP's repo (we were even considering that as an option this time).
|
@Mariatta LGTM, please merge when you're ready. Note that current master is ahead of the release branch (for example, master is Django 1.11, release is Django 1.7) so I made the release branch protected in order to prevent accidental outages. I can do the cherry-picking if you need this PR immediately deployed to production. |
|
Thanks @Naereen and @berkerpeksag. Does this get deployed automatically to staging.python.org? I don't have access to a computer where I can do the cherry-pick, I can do it later this evening. If you can get to it before me, it's fine too :) |
|
Can't do it either. Sorry. |
Some PEPs are using the .rst extension, and many are in .txt. The source link assumed that all the PEPs are using the .txt extension. With this change, it will use the correct file extension. Fixes python/peps#431
Yes, it does.
Done in 9235355. |
|
Thanks @berkerpeksag! Hmm the link to PEP 12 in staging.p.o is still incorrect though In staging.p.o, it still links to the .txt file. |
|
For some reason the peps repo on staging.p.o didn't get updated since the beginning of September. I will check it out. Production version works as expected: https://www.python.org/dev/peps/pep-0547/ |
4 participants
Some PEPs are using the .rst extension, and many are in .txt.
The source link currently assumes that all the PEPs are using
the .txt extension.
Fixes python/peps#431