-
-
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
PEP 12: Recommend and explain native RST hyperlinks (versus footnotes) #2302
PEP 12: Recommend and explain native RST hyperlinks (versus footnotes) #2302
Conversation
da2ec83
to
42e7d3b
Compare
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.
A question or two :)
pep-0012.rst
Outdated
|
||
In this paragraph, we refer to the `Python web site`_. | ||
|
||
An explicit target provides the URL. Put targets in a References | ||
section at the end of the PEP, or immediately after the reference. | ||
.. _Python web site: https://www.python.org/ |
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 line also be part of the RST code block so it's visible as an example?
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.
Yup, but actually, I do include it, in the appropriate example where I talk about link targets, which are discussed separately from link sources.
pep-0012.rst
Outdated
|
||
Visit the `website <https://www.python.org/>`__ for more. | ||
|
||
.. _Python web site: https://www.python.org/ |
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.
Is this line needed? It's not used in this example.
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.
Yup, good catch—this is a copy/paste error and was already included above. Oops!
pep-0012.rst
Outdated
|
||
For further examples, see the `documentation <pydocs_>`_. | ||
|
||
.. _pydocs: https://docs.python.org/ |
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.
Same question: should this line also be part of the RST code block so it's visible as an example?
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.
Like the other one, it is listed in the RST code block immediately below that discusses link targets.
If you intend to only reference a link once, and want to define it inline | ||
with the text, insert the link into angle brackets (``<>``) after the text | ||
you want to link, but before the closing backtick, with a space between the | ||
text and the opening backtick. You should also use a double-underscore after | ||
the closing backtick instead of a single one, which makes it an anonymous | ||
reference to avoid conflicting with other target names. For example: |
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.
Is it worth giving so much detail on how to write a link or to link to the reST docs at https://docutils.sourceforge.io/docs/user/rst/quickref.html#indirect-hyperlink-targets and provide the examples?
Same below for the other link styles.
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 agree -- I'd suggest most of this section could be replaced with a pointer to the Docutils reference, perhaps excepting not to use raw URLs and to prefer HTTPS.
I'd go so far as to remove the examples, too.
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.
My initial thought as well was just to point to the docutils reference or similar for the details, but:
- The other sections all explicitly describe how to use their syntax elements without reference to docutils, so we should be consistent (might be worth revising, but out of scope here)
- The docutils quickref doesn't actually explicitly discuss or provide examples for several of the things discussed here and commonly used/misused in PEPs, including indirect references of the form used here, multi-word link targets or the necessity of the space between the text and
<
, while the docutils full reference has a much higher level of comprehensive detail, much of which is not relevant here - This section contains a number of PEP-specific recommendations and guidance, such as where to put link targets, where and why to prefer different forms, and when to use anonymous references; the examples are tailored to PEPs; and the section is carefully organized to structure the key information in a logical and accessible manner for a PEP writer, rather than scattered all about.
So, my preference would be to keep it as-is, following the general status quo, and consider offloading the RST syntax details more generally in a potential future PR (after discussion on an issue). But if its overly lengthy, we could slim down some bits, like the rendered form for the different ways of specifying links.
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.
The detailed comments are from a time when we had to teach people how to write reST for a PEP since PEPs predate the format. Since reST is now so old, I don't think it's important long-term.
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 see, thanks for the historical background @brettcannon . I agree it would likely be a good idea to slim down the detailed reST syntax descriptions in PEP 12 to just a brief summary and/or any PEP-specific guidance, and link to the relevant docutils/Sphinx guides and documentation for the rest, rather than belaboring the PEP with such details that are described elsewhere.
That said, that's a distinct and more expansive change than just updating the existing descriptions of how to use hyperlinks and footnotes (as is the scope of #2130 that this resolves) and something we should probably discuss in a separate issue first to make sure we're all in agreement and decide on a consistent approach to this throughout the PEP before going ahead with it.
For now, I've reverted the addition of all the "renders as" example blocks and their associated text for the hyperlinks section, along with an extra paragraph and the explanation and examples of numbered footnotes. I suggest we go ahead and merge this with those changes for now, so the PEP accurately reflects the current recommendations for links/references as agreed on in #2130 (and so I can move ahead with #2266), and I can open a new issue for us to discuss and decide upon the approach to take with PEP 12 moving forward, and implement that once consensus is reached. Does that sound good to you?
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.
SGTM
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.
Thanks! Opened as #2337
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.
Looks good, some comments of varying triviality. I think we should reccomend listing links at the bottom of each section rather than all at the bottom for easier reviewing -- that would affect a few things in this PR.
A
pep-0001.txt
Outdated
12. Footnotes -- A collection of footnotes referenced in the PEP, and | ||
a central place to list non-inline hyperlink targets. |
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.
Wordsmithing -- should we avoid "referenced" to avoid confusion with inline references? Perhaps "used"?
"a central place to list non-inline hyperlink targets" -- do we want to still reccoment this? 682 which I just reviewed put links at the end of each section, which was actually quite nice in terms of reading the raw reST source. It doesn't matter from a rendering perspective but might make review easier?
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.
Wordsmithing -- should we avoid "referenced" to avoid confusion with inline references? Perhaps "used"?
Good point; though "used" isn't really accurate either, since the footnotes themselves aren't actually "used" in the PEP, but rather linked. "cited" is a good substitute, though, which I've replaced it with here as well as the template.
"a central place to list non-inline hyperlink targets" -- do we want to still reccoment [sic] this?
I wouldn't go so far as to say the current language "recommends" (assuming that's the word you mean) this approach, as opposed to simply offers it as an option; I removed "central" both places to put a little less empahsis on this.
I do recommend the "links after each paragraph" approach (that's much more proximate and predictable than after each section) for documentation in the Spyder Documentation Style Guide, but I don't think it makes sense to not at least offer a centralized list as an option, as most current PEPs use it, and it is very useful for PEPs that have large numbers of links or have many links with multiple uses (both of which made it invaluable for PEP 639), to list them in one common, known location.
So I'd favor not being overly prescriptive and breaking with current practice here.
The source for this (or any) PEP can be found in the PEPs repository, | ||
viewable on the web at https://github.com/python/peps/ . | ||
The source for this (or any) PEP can be found in the | ||
`PEPs repository <https://github.com/python/peps/>`_, |
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.
Can you also link the one on line 15 please?
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 guess so, though it is a bit out of scope here.
- Update your References and Copyright section. Usually you'll place | ||
your PEP into the public domain, in which case just leave the | ||
Copyright section alone. Alternatively, you can use the `Open | ||
Publication License`__, but public domain is still strongly | ||
preferred. | ||
|
||
__ http://www.opencontent.org/openpub/ |
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 only skimmed #2130, but I didn't see a copyright discussion -- there are a few PEPs licenced under the OPL (437, 483, 551, 578, 3145). For some authors there may be issues with PD/CC0, so noting the option exists may be useful?
Personally I am fine with PD (having released the code in this repo to it) -- and even better if I missed the dicussion & all good, but better safe than sorry & all that!
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.
An option doesn't exist; all PEPs must be dual-licensed CC0 and "PD" (which is effectively the same as just licensing CC0, which in turn is a public domain declaration) per PEP 1 as implemented in #1117 per the discussion with PSF legal in #123 ; the lack of a corresponding update to PEP 12 was presumably an oversight. The OPL (which itself is an ambiguous acronym, as it could refer to either the Open Content License or the Open Publication License, two different licenses) is an obsolete, ambiguous, incompatible and likely legally flawed license; it would be strongly preferable to get those old PEPs relicensed, but that is entirely out of scope here.
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.
Ahh, thanks for the links -- important that this gets updated, then.
A
If you intend to only reference a link once, and want to define it inline | ||
with the text, insert the link into angle brackets (``<>``) after the text | ||
you want to link, but before the closing backtick, with a space between the | ||
text and the opening backtick. You should also use a double-underscore after | ||
the closing backtick instead of a single one, which makes it an anonymous | ||
reference to avoid conflicting with other target names. For example: |
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 agree -- I'd suggest most of this section could be replaced with a pointer to the Docutils reference, perhaps excepting not to use raw URLs and to prefer HTTPS.
I'd go so far as to remove the examples, too.
pep-0012.rst
Outdated
Footnotes | ||
========= | ||
|
||
.. [1] Note that the footnote reference is a numbered one. | ||
|
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 think we should remove this bit and only recommend [#cite]
style footnotes, numbered is always a pain to update and leaves a large diff.
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.
Where "this bit" is the section that explains numbered footnotes
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 agree; I just didn't make that change out of conservatism, but I went ahead with it now.
if "references" in title_words or "footnotes" in title_words: | ||
# Remove references/footnotes section if there are no displayed |
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.
Ideally I'd use a set literal on the line above ({*section[0].astext().lower().split()}
instead of {*title_words}
) but GH won't let me put a suggestion on that line and also this line for reasons man has yet to understand.
if "references" in title_words or "footnotes" in title_words: | |
# Remove references/footnotes section if there are no displayed | |
if {"references", "footnotes"} & {*title_words}: | |
# Remove the references or footnotes sections if there are no displayed |
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, good point—I've manually made the change both here and in pep2html.py
.
GH won't let me put a suggestion on that line and also this line for reasons man has yet to understand.
I may or may not be a "man", but the reason, dumb as it is, is that intervening deleted lines that it doesn't like.
I had thought I'd left replies to most of the comments here a week ago, but it turns out they never got posted while I was on my trip down the SpaceX production and launch site in Texas that I got invited to. I'll go through them again and hopefully reply for real this time, now that I have some. |
fa28adb
to
78e97c7
Compare
78e97c7
to
be51524
Compare
This has been discussed a lot and everyone seems to be in agreement that this is generally a good idea, so I'll merge this now. Slimming down the reST primer and clarifying the role of PEP 1 and PEP 12 can be done in later PRs (I might have a go myself, time dependent). A |
As a note, it would probably be wise for us to gain consensus first on the issue, and to decide if and how we're going to break down and share the task, to avoid duplicate/conflicted effort and wasting our always-valuable limited volunteer bandwidth. |
As a followup to #2155 , which stops auto-rendering link targets as footnotes, and hides the References section if only link targets are used (as well as #2227 and #2229 which fixed bugs with its initial implementation), and #2291, which made initial changes to PEP 12 to recommend the new PEP and RFC roles instead of footnotes for referencing PEP, this PR finally finishes implementing #2130 by further updating PEP 12 to recommend using native RST hyperlinks over manual footnotes for referencing URLs, as well as better explaining how to do so.
This PR also changes the suggested title from "References" to "Footnotes" to reflect its use and contents, updates a couple lines in PEP 1 and the template that reference the old use, and tweaks a few links in PEP 0, PEP 1, PEP 12 to follow the new recommendations, as these PEPs are highly-active meta-PEPs and exemplars and templates for the others.
While there seemed to be general consensus among the PEP editors on my suggested course of action on #2130, including the revised suggested section title to be more appropriate to its contents (as already used on some PEPs), I specifically made that as a separate commit, so if for whatever reason, we don't want to change it for new PEPs, I should be able to (more or less) cleanly drop that commit.
(Finally) closes #2130