Skip to content

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

Closed
wants to merge 8 commits into from

Conversation

Copy link
Member

@corona10 corona10 left a 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
Copy link
Member

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.

@bedevere-bot
Copy link

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@corona10
Copy link
Member

@serhiy-storchaka
Can you please take a look?
I am not sure that we can accept the change of existence HTTP status code.

@corona10 corona10 changed the title gh-102247 Update HTTP status codes in http package to match rfc9110 gh-102247: Update HTTP status codes in http package to match rfc9110 Mar 10, 2023
@@ -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'
Copy link
Member

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

Copy link
Member

@corona10 corona10 Mar 10, 2023

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.

Copy link
Contributor

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',
Copy link
Member

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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -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)
Copy link
Member

@corona10 corona10 Mar 10, 2023

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

cpython/Lib/enum.py

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.

Copy link
Contributor Author

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.

@corona10 corona10 requested a review from ethanfurman March 10, 2023 14:43
@corona10
Copy link
Member

@ethanfurman

It looks like _test_simple_enum doesn't support alias cases.
Do you have any recommended way or do we have to fix _test_simple_enum to support the alias case?

cpython/Lib/enum.py

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.

Copy link
Member

@corona10 corona10 left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants