-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 1: Make text/x-rst officially the default and update accordingly #2355
PEP 1: Make text/x-rst officially the default and update accordingly #2355
Conversation
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 have a few questions about the status of text/plain
. I also wonder if it would be helpful to include a "Changes" section in the PEP. Yes, you can dig it out of the git history, but that's a pain.
FWIW, +1 on the switch to text/x-rst
as the default. Thanks for all your great work on clean up the PEPs!
Content-Type header is present. | ||
All PEPs now use reStructuredText (see :pep:`12`), | ||
and have a value of ``text/x-rst``, the default. | ||
Previously, plaintext PEPs used ``text/plain`` (see :pep:`9`). |
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.
This should clearly explain whether text/plain
will be accepted or not going forward.
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'd remove this line, PEP-0009 itself has a notice stating that it is withdrawn.
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.
If people think this should simply be removed, I can remove it. Otherwise, I could revise the above to say "All PEPs must use reStructuredText"...
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.
Yes, I think the must language is worth adding.
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.
Added above, thanks
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.
Suggested changes make it much more explicit that only text/x-rst
is allowed.
pep-0001.txt
Outdated
All new and active PEPs must use reStructuredText, but for backwards | ||
compatibility, plain text is currently still the default if no | ||
Content-Type header is present. | ||
All PEPs now use reStructuredText (see :pep:`12`), |
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.
Remove now, and the reference to PEP 12. PEP 9 does not follow the reST template in PEP 12, but does use reST.
All PEPs now use reStructuredText (see :pep:`12`), | |
All PEPs use reStructuredText, |
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 removed now "now" and "must" per @warsaw 's suggestion. Unless there's something I'm misunderstanding, it seems rather silly to remove the reference to PEP 12 just because one withdrawn PEP doesn't include the suggested; in truth, many PEPs don't to varying extents, but they still use the standard headers and valid reST syntax (as does PEP 9).
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.
Maybe "(see :pep:`12` for the reStructuredText template)"? It didn't feel particularly linked, but equally is rather minor.
@@ -603,11 +603,9 @@ The Type header specifies the type of PEP: Standards Track, | |||
Informational, or Process. | |||
|
|||
The format of a PEP is specified with a Content-Type header. |
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.
Explicitly only support text/x-rst
The format of a PEP is specified with a Content-Type header. | |
The format of a PEP is specified with a Content-Type header. | |
The only valid value is ``text/x-rst``. |
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 ended up using the "must" verbiage suggested by @warsaw
Content-Type header is present. | ||
All PEPs now use reStructuredText (see :pep:`12`), | ||
and have a value of ``text/x-rst``, the default. | ||
Previously, plaintext PEPs used ``text/plain`` (see :pep:`9`). |
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'd remove this line, PEP-0009 itself has a notice stating that it is withdrawn.
@@ -6,6 +6,7 @@ PEP-Delegate: <PEP delegate's real name> | |||
Discussions-To: <email address or URL> | |||
Status: <REQUIRED: Draft | Active | Accepted | Provisional | Deferred | Rejected | Withdrawn | Final | Superseded> | |||
Type: <REQUIRED: Standards Track | Informational | Process> | |||
Content-Type: text/x-rst |
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.
Content Type is required, but as there's only one valid value it feels odd to use the <REQUIRED: spam>
verbiage. Maybe it is better as is?
Content-Type: text/x-rst | |
Content-Type: <REQUIRED: text/x-rst> |
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.
Why is Content-Type
still required?
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 had the same thought. One possible reason is forward compatibility: maybe in the future we'll allow PEPs in markdown or something. Then again, even in that case we can just reintroduce the header later.
And another reason is just to avoid making a change without a strong reason. There is little cost to having PEP authors copy this field from the template.
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'm inclined to keep it, for clarity. Lots of things have their MIME types designated in headers. And as Jelle says, maybe in the future there will be others, so it'd mean we don't need to say "Optional - if omitted, defaults to text/x-rst".
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.
There's no techincal reason the header couldn't simply be removed from the template (and even all PEPs), since they all use text/x-rst
, the default, and likely will for the immediate future, and the only place this was actually used is by the old docutils-based rendering script to determine the rendering mode, if not the default (it isn't anywhere in the output, and it isn't needed or used at all by the PEP 676 based system).
If we did add support for a new format, it presumably would be handled via file extension, like any common format these days (e.g. MyST with .md
) and a Content-Type
would not really serve no technical purpose, since it is only (and could even be out of sync with the actual file type, unless we run a linter or custom build-time check to validate it). That would allow us to not have to mention it at all (optional or otherwise), lint it, or copy it as boilerplate.
That being said, while it is redundant, there isn't too much cost to keeping it around (at least on existing PEPs), so I don't feel too strongly about it.
3aa9d67
to
51cf8cb
Compare
@CAM-Gerlach are you happy for this to be merged? // are there any remaining blockers? A |
Nope, thanks, not from my end, and not that I see here. My intent was to discuss what to do about the |
As mentioned in #2352 , despite every single PEP in the repo, even PEP-0009, using
Content-Type: text/x-rst
, the obsolete plain text format described in said PEP, withContent-Type: text/plain
, is still the default (at least in the old docutils-based system; this is fixed in the new PEP 676 Sphinx-based one).Therefore, this flips the default to
text/x-rst
, and updates PEP 1 and the reST PEP template accordingly (which up until now, lacked a Content-Type header entirely, meaning that PEPs would in fact render as plain-text). This makes the Sphinx and docutils build systems consistent, allows simplifying the Readme, Contributing Guide, PEP 1 and the template, and sets the stage for eliminating this now-redundant field entirely in the future (e.g. if/when acceptance of PEP 676).