-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-93166: Clarify http.server documentation using RFC 7230 terminology #30686
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
Conversation
|
A nice piece of work to tidy this up. Only minor changes please: Line 274: Adding |
|
This is way too large a change to pass as skip issue. Please file an issue for this change. |
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 made a few notes on the first changes in the PR.
CPython is big and old, and as we’ve discussed in other PRs, sweeping changes are weighed cautiously. There is also no goal to apply a very specific style to all parts of the documentation; it’s fine if different documents contributed by different authors may have different styles, as long as it fits in its overall doc section.
To apply your time and energy in a way that works for CPython reviewers and gives more satisfactory results for you, I would recommend that you engage more before making edits: create tickets with specific points to be improved, and/or discuss these points on discuss. You could even join the Documentation community team to take part in tackling the big problems in Python’s doc! https://docs-community.readthedocs.io/en/latest/community/index.html
| @@ -24,7 +24,7 @@ This module defines classes for implementing HTTP servers. | |||
|
|
|||
| One class, :class:`HTTPServer`, is a :class:`socketserver.TCPServer` subclass. | |||
| It creates and listens at the HTTP socket, dispatching the requests to a | |||
| handler. Code to create and run the server looks like this:: | |||
| handler. Code to create and run the server looks like this:: | |||
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.
Please avoid this kind of changes. They do not fix or improve anything, so just churn history for not benefit.
|
|
||
| .. attribute:: path | ||
|
|
||
| Contains the request path. If query component of the URL is present, | ||
| The request target. If query component of the URL is present, |
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.
Let’s not replace path with target 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.
It is not the path component of a URI whose syntax is defined in RFC 3986, it is the request-target component of an HTTP request line whose syntax is defined in RFC 7230. The other documented attributes command (method) and request_version (HTTP-version) of BaseHTTPRequestHandler already use the syntactic terminology of an HTTP request line.
| then ``path`` includes the query. Using the terminology of :rfc:`3986`, | ||
| ``path`` here includes ``hier-part`` and the ``query``. | ||
|
|
||
| .. attribute:: request_version | ||
|
|
||
| Contains the version string from the request. For example, ``'HTTP/1.0'``. | ||
| The request HTTP version. For example, ``'HTTP/1.0'``. |
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.
Previous wording was more elegant (except contains)
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 am using RFC 7230 terminology.
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 also think the original version is more readable and is consistent with RFC, and provides additional information that it is a string.
@erlend-aasland Done. |
|
Please remove all changes not directly related to the issue, as previously requested by Éric in #30686 (review). |
|
I've marked this PR as a draft since it is not ready for review. |
|
Closing due to inactivity. If you'd like to have this change merged, please update per Éric and Erlend's requests. A |
Uh oh!
There was an error while loading. Please reload this page.