PEP 676: Refine formatting, spelling and syntax, and copyedit text #2200
Conversation
|
Hi Chris -- not sure if you saw my comment at the bottom of # 2173, so this is just a holding note to say thanks & that I'll review this on Tues or Wed (UK time, so translate that to the US!). |
pep-0676.rst
Outdated
| a tar archive to a server, where it is retrieved and rendered into the | ||
| multi-system, multi-stage process. A Continuous Integration (CI) task runs a | ||
| `docutils`_ script to render all PEP files individually. The CI task then | ||
| uploads a TAR to a server, where it is retrieved and rendered into the |
There was a problem hiding this comment.
I'd also keep this as "tar archive".
The word "tar" is commonly written in lowercase: https://en.wikipedia.org/wiki/Tar_(computing)
(It's originally from "tape archive, so not strictly an initialism.)
So archive in "tar archive" is kind of redundant, but it's very common to write "tar archive" and I feel it's clearer, and, well, we're not dealing with tapes.
| uploads a TAR to a server, where it is retrieved and rendered into the | |
| uploads a tar archive to a server, where it is retrieved and rendered into the |
There was a problem hiding this comment.
You're correct on the lowercase, of course (its an extension and a CLI program, not an acronym), and as much as the redundancy irks me, I can see the clarity benefit (for which reason I almost backed out the change originally), and I'm firmly in the camp that clarity trumps aesthetics, purity and other considerations if they are in conflict and cannot be resolved through other means.
There was a problem hiding this comment.
Pedantic side note @hugovk — have I been pronouncing tar (the archive) wrong all this time? As an acronym in the strict sense of pronunciation ("tar", one word) rather than an initialism ("t-a-r", single letters)?
On a related side-side note, when I was younger I used to insist on pronouncing my name as an initialism ("C-A-M") rather than an acronym ("Cam"), since they were technically my first three initials after all, but everyone called me "CAM" anyway, so I eventually just embraced it.
There was a problem hiding this comment.
I have only heard and say "tar" as one word, but who knows!
"CAM" instead of "C-A-M": people say "NASA" rather than "N-A-S-A" :)
There was a problem hiding this comment.
I have only heard and say "tar" as one word, but who knows!
Yeah, that's all I've heard too haha; my (pedantic) comment was based on your reference to it as an initialism rather than an acronym, heh.
"CAM" instead of "C-A-M": people say "NASA" rather than "N-A-S-A" :)
And I should know, since I work there :)
There was a problem hiding this comment.
@CAM-Gerlach -- I've been addressing you as Chris / Christopher per your logo-name thing -- would you prefer Cam?
A
There was a problem hiding this comment.
Yeah, sure :) While "CAM" is in the center of my logo-name thing, you're not the first person to look at it closely and see the "Christopher" instead, heh
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
CAM-Gerlach
force-pushed
the
polish-pep-676
branch
from
21c4474
to
6263951
Compare
There was a problem hiding this comment.
Comments below -- most are my personal opinion and on style issues I have no authority other than what scans to me and what doesn't!
The below diff is all my revisions to your suggestions versus current HEAD.
A
(and thanks again, I'm aware text review isn't the most exciting thing in the world so I do appreciate you taking the time 🙂)
diff --git a/pep-0676.rst b/pep-0676.rst
index da3545d74..2c03797c7 100644
--- a/pep-0676.rst
+++ b/pep-0676.rst
@@ -7,7 +7,7 @@ Status: Draft
Type: Process
Content-Type: text/x-rst
Created: 01-Nov-2021
-Post-History: 23-Sep-2021
+Post-History: 23-Sep-2021, 30-Nov-2021
Abstract
========
@@ -53,9 +53,9 @@ to create a full local instance of the `python.org`_ website and run a number
of disparate scripts, following documentation_ that lives outside of the
`python/peps`_ repository.
-The proposed implementation provides a single Makefile_ and a Python script to
-render all PEP files, with options to target a web-server or local filesystem
-environment.
+By contrast, the proposed implementation provides a single Makefile_ and a
+Python script to render all PEP files, with options to target a web-server or
+the local filesystem.
Using a single repository to host all tooling will clarify where to raise
issues, reducing volunteer time spent in triage.
@@ -82,30 +82,34 @@ example, the reference implementation no longer requires a scheduled render
process to run, instead initiating rendering on every commit to the
`python/peps`_ repository.
-Equally, there are a small number of broken items, for example list styles not
-being respected or support for updating images being challenging with the
-system [7]_. These would be solved by default in the proposal.
+The reference implementation also fixes several small issues, for example:
+* list styles are currently not respected [7]_
+* support for updating images being challenging with the system [7]_
-Commercial providers such as `Read the Docs`_ can additionally enhance this
+Third-party providers such as `Read the Docs`_ can additionally enhance this
experience, for example by providing rendered previews of changes in pull
requests.
Specification
=============
+.. note:: The key words "MUST", "SHOULD", and "MAY" in this section are used
+ as per :rfc:`2119`.
+
The proposed specification for rendering the PEP files to HTML is as per the
`reference implementation`_.
-That the HTML files should be made available under peps.python.org_. The
-rendered files may be hosted just as static files, and behind a content
-delivery network (CDN).
+The rendered PEPs MUST be available at peps.python.org_. These SHOULD be
+hosted as static files, and MAY be behind a content delivery network (CDN).
+
+The following redirect rules MUST be created for the `python.org`_ domain:
-The following redirect rules must be created from the `python.org`_ domain:
+* ``/peps/`` -> https://peps.python.org/
+* ``/dev/peps/`` -> https://peps.python.org/
+* ``/peps/(.*)\.html`` -> https://peps.python.org/$1
+* ``/dev/peps/(.*)`` -> https://peps.python.org/$1
-* /peps/ -> https://peps.python.org/
-* /dev/peps/ -> https://peps.python.org/
-* /peps/(.*)\.html -> https://peps.python.org/$1
-* /dev/peps/(.*) -> https://peps.python.org/$1
+The following nginx configuration would achieve this:
.. code-block:: nginx
@@ -121,21 +125,23 @@ The following redirect rules must be created from the `python.org`_ domain:
return 308 https://peps.python.org/;
}
-Redirects must be implemented to preserve `URL fragments`_ for backward
-compatability purposes.
+Redirects MUST be implemented to preserve `URL fragments`_ for backward
+compatibility purposes.
Backwards Compatibility
=======================
-Due to server-side redirects to new canonical URLs, there are no backwards
-compatability concerns. Links in previously published materials referring to
-any old URL scheme will be guaranteed to work.
+Due to server-side redirects to new canonical URLs, links in previously
+published materials referring to old URL schemes will be guaranteed to work.
+We have not identified any other backwards compatibility concerns.
Security Implications
=====================
-No security implications, and the main `python.org`_ website will no longer
-take raw HTML uploads, closing a potential threat vector.
+The main `python.org`_ website will no longer process raw HTML uploads,
+closing a potential threat vector. PEP rendering and deployment processes will
+use well maintained modern code and secure automated platforms, further
+reducing the potential attack surface. We see no negative security impact.
How to Teach This
=================
@@ -160,9 +166,8 @@ issue mitigations mentioned above. However, we do not believe that this would
solve the distributed tooling issue.
It would be possible to use the output from the proposed rendering system and
-import it into `python.org`_. We would argue however that this would be the
-worst of both worlds, as a great deal of complexity is added, and none is
-removed.
+import it into `python.org`_. We would argue that this would be the worst of
+both worlds, as a great deal of complexity is added whilst none is removed.
Open Issues
===========| @@ -9,20 +9,22 @@ Content-Type: text/x-rst | |||
| Created: 01-Nov-2021 | |||
| Post-History: 23-Sep-2021 | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Whilst PEP 12 does technically say that two blank lines are required, it also calls for e.g. double spacing, which a substantial number of PEPs ignore. I would argue it's a stylistic choice when editing documents, and would just add to git blame noise so I'd drop this (& all other similar line insertions).
There was a problem hiding this comment.
I'm not sure I've seen any recent PEP use 2 spaces after periods, and that convention is very uncommon in in general these days (much more so than when PEP 9/PEP 12 was written) so maybe PEP 12 should be updated on that point—I can open an issue after I finish my PR updating the references/footnotes wording. But as for two blank lines before headings, I find that as more of a readability issue, not just a style one, as I find it much easier to clearly distinguish section breaks, and see that convention or a similar one much more frequently in Markdown and reST elsewhere.
That said, I don't feel strongly about it and don't want to override your preference as the PEP author, though I'd rather hear from @JelleZijlstra or another PEP editor before going back through and redoing it.
| We propose that PEPs are accessed through peps.python.org_ at the top-level | ||
| namespace (for example `peps.python.org/pep-0008/`_), and that all custom | ||
| We propose that PEPs are accessed through `peps.python.org`_ as the top-level | ||
| namespace (for example, `peps.python.org/pep-0008`_), and that all custom | ||
| tooling to support rendering PEPs is hosted in the `python/peps`_ repository. |
There was a problem hiding this comment.
Same as above, I'd drop these (the trailing forward slash might be unneeded, but its a small thing that signifies it's a directory not a file)
There was a problem hiding this comment.
I actually dropped the trailing forward slash to—in the spirit of the PEP's strong lack of specificity elsewhere in seemingly more important areas, such as not even mentioning the name of the new build backend (Sphinx)—abstract away what I saw as an implementation detail. But I defer to your judgement on that.
I can understand the stylistic choice of not putting explicit backticks around single-word references, so long as its consistently, but what originally prompted this change was the fact that you do put backticks around `python.org`_, but don't put them around peps.python.org_, which doesn't ,seem to make any sense. At the very least, they should be used around both, or neither.
| @@ -33,15 +35,15 @@ This PEP provides a specification for self-contained rendering of PEPs. This | |||
| would: | |||
|
|
|||
| * reduce the amount of distributed configuration for supporting PEPs | |||
| * enable quality-of-life improvements for those who read, write, and review | |||
| PEPs | |||
| * enable quality-of-life improvements for those who read, write and review PEPs | |||
There was a problem hiding this comment.
After the change this line is 80 characters long -- one bit of PEP 12 I did follow is to keep to 79 maximum for prose!
There was a problem hiding this comment.
I only measure it as 79, like a number of other lines, unless you could the newline (which I thought didn't count?) But maybe I'm mistaken.
| `docutils`_ script to render all PEP files individually. The CI task then | ||
| uploads a tar archive to a server, where it is retrieved and rendered into the |
| and maintainable solution for PEP readers, authors, and editors. | ||
|
|
||
|
|
There was a problem hiding this comment.
See above (xref extra lines)
| requests. | ||
|
|
||
|
|
||
| Specification | ||
| ============= | ||
|
|
There was a problem hiding this comment.
We should note that we are using MUST/SHOULD/MAY as per BCP 14 / RFC 2119
| .. note:: The key words "MUST", "SHOULD", and "MAY" in this section are used | |
| as per :rfc:`2119`. |
There was a problem hiding this comment.
They are widely used in the specification section of other PEPs, and I've never seen it explicitly noted like this. Perhaps it would be a better idea, if this is desired, to do so in PEP 1 or PEP 12 instead, instead of redundantly including this note in every PEP.
|
|
||
| The following redirect rules must be created from the `python.org`_ domain: | ||
| The following redirect rules MUST be created for the `python.org`_ domain: |
There was a problem hiding this comment.
No strong feelings, I supose I'd argue the redirect rules are being created from python.org to peps.python.org, although the purpose is for peps.python.org to work.
There was a problem hiding this comment.
It originally had "from" and I hadn't changed it, but @hugovk made the point that what was being described being "created" are the redirect rules, which are for the python.org domain, while the redirects themselves are from that domain to peps.python.org
There was a problem hiding this comment.
Ah, I see. Both are fine then, depending on how you read it :)
I was reading it like instructions for the python.org administrator who must make rules for that domain. No strong objection either way, it's probably clear enough either way.
| By contrast, the proposed implementation provides a single `Makefile`_ and a | ||
| Python script to render all PEP files, with options to target a web server or | ||
| the local filesystem environment. |
There was a problem hiding this comment.
| By contrast, the proposed implementation provides a single `Makefile`_ and a | |
| Python script to render all PEP files, with options to target a web server or | |
| the local filesystem environment. | |
| By contrast, the proposed implementation provides a single Makefile_ and a | |
| Python script to render all PEP files, with options to target a web-server or | |
| the local filesystem. |
| Equally, there are a number of broken items currently; for example, list styles | ||
| are not respected, and support for updating images is challenging with the | ||
| current system [7]_. These are solved by the reference implementation of | ||
| this proposal. |
There was a problem hiding this comment.
| Equally, there are a number of broken items currently; for example, list styles | |
| are not respected, and support for updating images is challenging with the | |
| current system [7]_. These are solved by the reference implementation of | |
| this proposal. | |
| The reference implementation also fixes several small issues, for example: | |
| * list styles are currently not respected [7]_ | |
| * support for updating images being challenging with the system [7]_ |
There was a problem hiding this comment.
I like the simplification. However:
-
It doesn't make much sense to use, and many style guides explicitly prohibit, an unordered list for only two items (three is typically the minimum), without strong justification. It would be ideal to add a third item if you have one, but otherwise the listification would best be reverted.
-
Furthermore, both items in the list reference the same footnote, which is redundant and doesn't really make sense, so that should either go next to "several small issues", or perhaps better, convert the footnote to individual direct inline links.
-
Finally, some of the wording changes that were avoided were important to avoid unnecessarily minimizing and trivializing the fixes, making a more compelling case for the PEP. If you don't like the particular words chosen, I can certainly suggest others that would still avoid this issue.
There was a problem hiding this comment.
Hmm,
-
I think two bullet points are just fine, especially if it helps lay out the information more clearly and improves readability (people don't read screens, they scan)
-
I'd be interested to see what style guides prohibit two, the first I found talking about bullet points has several with just two! And Google's style guide is usually well thought out, they also use and don't prohibit two.
| Due to server-side redirects to new canonical URLs, links in previously | ||
| published materials referring to the old URL scheme will be guaranteed to work. | ||
| Furthermore, aside from the resolved bugs and new implemented features, all | ||
| existing PEPs will still render as they did before with the new backend. | ||
| Therefore, this poses no major backwards compatibility concerns. |
There was a problem hiding this comment.
will still render as they did before with the new backend
You can argue this both ways (the outputted HTML is different, but the words are the same). I'm not sure it adds anything, I guess.
| Due to server-side redirects to new canonical URLs, links in previously | |
| published materials referring to the old URL scheme will be guaranteed to work. | |
| Furthermore, aside from the resolved bugs and new implemented features, all | |
| existing PEPs will still render as they did before with the new backend. | |
| Therefore, this poses no major backwards compatibility concerns. | |
| Due to server-side redirects to new canonical URLs, links in previously | |
| published materials referring to old URL schemes will be guaranteed to work. | |
| We have not identified any other backwards compatibility concerns. |
There was a problem hiding this comment.
While I understand my original wording wasn't the best, I have to disagree with your dismissal of its importance overall. As a reader and reviewer of the PEP, my biggest backward compat concern aside from the URL change would be whether it causes any major rendering regressions in displaying previously valid syntax/formatting used in existing PEPs. Therefore, I certainly wouldn't dismiss this entirely as a backward compat concern. While those of us involved are well aware it doesn't, making this explicit helps ensure this is clear to everyone. What about (also fixed a missing definite article):
| Due to server-side redirects to new canonical URLs, links in previously | |
| published materials referring to the old URL scheme will be guaranteed to work. | |
| Furthermore, aside from the resolved bugs and new implemented features, all | |
| existing PEPs will still render as they did before with the new backend. | |
| Therefore, this poses no major backwards compatibility concerns. | |
| Due to server-side redirects to the new canonical URLs, links in previously | |
| published materials referring to the old URL scheme will be guaranteed to work. | |
| Furthermore, while the theme and presentation may differ, all existing PEPs | |
| will still render correctly as they did before with the new backend. | |
| Therefore, this poses no major backwards compatibility concerns. |
|
Sorry. If I'd known my consistency and formatting tweaks would have been this controversial and resulted in dozens and dozens of individual detailed comments, I would have (and perhaps should have) thought better of suggesting them in the first place, since I'd consider both your and my time and effort bikeshedding over and applying/reverting these nits to be much more valuable than either a cleaner If you want, maybe it would be best if you just go ahead and apply your patch with any of my followup corrections/suggestions you'll accept, and I'll close this PR. I didn't mean to cause so much trouble for both of us, and I'll be more careful next time about suggesting changes other than grammar or meaningful alternations to the text, and leave the rest to @JelleZijlstra and the other PEP editors. |
|
Firstly an apology -- I really value your suggestions and review, and I hope I haven't offended you. Some context -- I did this review straight after my final exam for the winter, as I was perhaps too anxious to get back to you. In retrospect this (review straight after exam) was a terrible idea, as I was still very much in a detail orientated mindset etc. Sorry for this. I also didn't realise that each comment would be sent out individually over email (which I realised after your follow-up review!) -- I haven't used the review features of GH much before, I imagined that all the comments would be rolled up into one email or there would simply be a alert that there had been a new review. Sorry for this too. I think we have also been approaching this from different perspectives, and potentially different backgrounds. My history in technical writing is one where plain English & higher level details are the most important -- winning over potentially more informal grammar, or ambiguity in the edge cases (for example: "a lot" over "many"). I think (do correct me) that you're approaching this from a standards background, where zero ambiguity is the most important. This of course is more relevant for the PEPs as a corpus. (I also believe our interpretations of the word 'consistency' were initally different -- I was thinking about intra-document consistency, and you were going for consistency with the median PEP). The genesis of PEP 676 lends itself initially to more my background -- it was initially a very high level summary of the build system, then fleshed out into a slightly more detailed description & rationale when posted to Discourse, then fleshed out further into the current text when I was asked to formalise it into a PEP. However as a PEP you're right to hold it to a higher standard of technical writing, so I want to achieve that standard. (I will also admit some of my comments above were trying to maintain what I perceived as a consistentent tone of voice [i.e. mine :P] throughout the document. Pride before the fall and all that...) I don't find your suggestions controversial I want to note -- and sorry if my review notes came across that way. With all this in mind: Potential way forwards: Entirely recognise your point about effort/reward and sunk cost (especially as a student of economics!). I'm thus not going to go through and respond to all the points inline, as much as it is tempting. I've thumbs-up-ed various things -- the ones I haven't are as markers that I need to read it again. I think in the context of consistency with the body of PEPs, the points you made about double blank lines before section headers and links enclosed in grave accents make sense. I'm planning to go through later today and apply your changes on top of my local version (that I made the diff from). I'm also going to flesh out the reference implementation section to say that we propose using Sphinx! Would it be best if I made a PR against this branch on your A |
|
Thanks both for your efforts, none further comments from me :) @AA-Turner GitHub has a really nice feature for grouping all review notes in a review. I've made a quick demo at hugovk/test#37, want to head over and try it out? That's my playground repo for learning and experimenting with GitHub/Git, anything goes! |
No worries—I was a bit frustrated, but at myself, for being too detail-oriented on my own part in both my original changes and my replies. I appreciate you taking the time to respond in detail, though I certainly regret you having to spend it.
Hey man, I do this exact same sort of thing a lot more often then I'd like to admit...I can totally understand you there. And my initial proposed changes were rather overly detail-oriented to begin with.
Yeah, my approach to this was from the perspective of a formal technical/specification document, and following the general tone, language and style of the other PEPs; I'd take a somewhat less formal tone when, e.g. working on documentation, and far less so on, e.g., a blog post (and particularly far less invasive changes toward anything that might meaningfully affect the author's voice). I do want to note, though, that consistency within the PEP is at least as important as consistency with other PEPs (which was the justification for, e.g. using backticks consistently across e.g. URLs), and I hope I didn't harm that too much. Language like "a lot" might not be the most appropriate in a specification, but conversly in other contexts, overly formal constructs might be equally inappropriate.
Heh, well it is important that you retain your author's voice, though perhaps less so in a PEP (particularly the specification and other normative sections), than, say a blog post, but perhaps more so than documentation for a third party project. It's worth sticking up for, and I would do the same in your place.
That's what I would have suggested, and looks like that's what you just did. I've got to get on a flight now, but I'll try to look it over tonight, or early tomorrow. Though from what I looked at, it all looks pretty good to me. Thanks! As such, I'll close this one.
Actually, @hugovk , I believe @AA-Turner did it right, consolidating his comments and suggested changes into a single review (and using suggested changes where practical, etc); I think the problem was when I replied, those all went out as individual emails, which I could potentially have avoided that by leaving my own replies all as a "review", but this results in the reply comments being duplicated twice on the PR, and leads to some very weird and potentially confusing UI/UX. Unfortunately, I I'm not sure there's a better way around this using GitHub's built-in tools, without collaborating externally. |
|
Superceded by #2207 |
@AA-Turner
In addition to the changes here, I'm still a little concerned that readers will come away from the PEP not understanding what it actually proposes doing, specifically, as opposed to a more abstract set of principles. While I can understand the desire to not hard-codify the exact tool used, I believe it would certainly be worth at least mentioning a few key details of the proposed reference implementation, e.g. Sphinx will be used as the build system, as it is that which allows for many of the stated improvements. But we can discuss that further on the issue, if needed.