-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-102247: Update HTTP status codes in http package to match rfc9110 #102570
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
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.
You have to notify the change to the whatsnews
Please refer to the similar commit.
corona10@a8e9348
``421`` ``MISDIRECTED_REQUEST`` HTTP/2 :rfc:`7540`, Section 9.1.2 | ||
``422`` ``UNPROCESSABLE_ENTITY`` WebDAV :rfc:`4918`, Section 11.2 | ||
``421`` ``MISDIRECTED_REQUEST`` HTTP Semantics :rfc:`9110`, Section 15.5.20 | ||
``422`` ``UNPROCESSABLE_CONTENT`` HTTP Semantics :rfc:`9110`, Section 15.5.21 |
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.
You have to notify the change with the versionchanged tag.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@serhiy-storchaka |
@@ -130,7 +131,7 @@ def is_server_error(self): | |||
'Server refuses to brew coffee because it is a teapot.') | |||
MISDIRECTED_REQUEST = (421, 'Misdirected Request', | |||
'Server is not able to produce a response') | |||
UNPROCESSABLE_ENTITY = 422, 'Unprocessable Entity' | |||
UNPROCESSABLE_CONTENT = 422, 'Unprocessable Content' |
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.
One of the problems of removing this constant is that it will break end-user code.
This is one example
https://github.com/aiven/karapace/blob/88bee3ab15115fca80b8f74020ebb30a3b562ffe/karapace/karapace.py#L41-L46
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.
Even if we decide to accept this change, we may need to add deprecation period rather than direct removing.
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 have to keep the old names as aliases for the new ones. I would continue to support them indefinitely, even -- deprecations and breaking things has a cost, and supporting a few extra constant names indefinitely is essentially free, so it's not worth spending our limited breakage budget on it.
@@ -1688,14 +1688,14 @@ def test_client_constants(self): | |||
'GONE', | |||
'LENGTH_REQUIRED', | |||
'PRECONDITION_FAILED', | |||
'REQUEST_ENTITY_TOO_LARGE', |
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 maintain old constants
'REQUEST_URI_TOO_LONG', | ||
'UNSUPPORTED_MEDIA_TYPE', | ||
'REQUESTED_RANGE_NOT_SATISFIABLE', | ||
'EXPECTATION_FAILED', | ||
'IM_A_TEAPOT', | ||
'MISDIRECTED_REQUEST', | ||
'UNPROCESSABLE_ENTITY', |
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.
ditto
Lib/test/test_httplib.py
Outdated
@@ -525,7 +525,8 @@ def test_dir_with_added_behavior_on_status(self): | |||
self.assertTrue({'description', 'name', 'phrase', 'value'} <= set(dir(HTTPStatus(404)))) | |||
|
|||
def test_simple_httpstatus(self): | |||
class CheckedHTTPStatus(enum.IntEnum): | |||
@enum._simple_enum(enum.IntEnum) |
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 don't think that this change is the proper approach to fixing the test suite issue.
see
Lines 1896 to 1913 in 53dceb5
def _test_simple_enum(checked_enum, simple_enum): | |
""" | |
A function that can be used to test an enum created with :func:`_simple_enum` | |
against the version created by subclassing :class:`Enum`:: | |
>>> from enum import Enum, _simple_enum, _test_simple_enum | |
>>> @_simple_enum(Enum) | |
... class Color: | |
... RED = auto() | |
... GREEN = auto() | |
... BLUE = auto() | |
>>> class CheckedColor(Enum): | |
... RED = auto() | |
... GREEN = auto() | |
... BLUE = auto() | |
>>> _test_simple_enum(CheckedColor, Color) | |
If differences are found, a :exc:`TypeError` is raised. |
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 was wondering about the different use of enum in HTTPStatus and CheckedHTTPStatus, so I'm going to change it back.
It looks like Lines 1896 to 1913 in 53dceb5
|
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 revert #102570 (comment)
and wait @ethanfurman 's advice.
This reverts commit 90bf421.
References