-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-91996: Adding an HTTPMethod StrEnum to http #91997
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
Every change to Python requires a NEWS entry. Please, add it using the blurb_it Web app or the blurb command-line tool. |
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 update the documentation if you want to add this enum
https://docs.python.org/3/library/http.html?highlight=httpstatus#http.HTTPStatus
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 |
Lib/http/__init__.py
Outdated
obj.description = description | ||
return obj | ||
|
||
GET = 'GET', 'Transfer a current representation of the target resource.' |
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.
These should be in some sort of order, probably alphabetical, possibly grouped by safety: GET
, HEAD
, OPTIONS
/ DELETE
, PATCH
, POST
, PUT
/ CONNECT
, TRACE
.
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 used the same order from the RFCs, this is the usual order they are sorted (e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods)
While it's not technically a standard yet, would be good to be aware of https://www.ietf.org/archive/id/draft-ietf-httpbis-safe-method-w-body-02.html, the This also misses any WebDAV methods, which users of Flask/Werkzeug occasionally ask about. If someone used this type to annotate a parameter, it would prevent users from using other methods unless the annotation was |
Also, consider the churn that will be introduced by this. I guarantee that some user of every library that uses HTTP will open issues asking for the library's methods to support these, which will then require every maintainer to evaluate whether they want to support it or keep rejecting requests to support it forever. Since all existing libraries use strings, they will have to add more complex code to accept both. |
I think it might be better to include the QUERY method in a seperate PR since it isn't part of the http standard and might change over time.
Most of the users don't really know about webDAV and it might be confusing to them to see it, since HTTPMethod is such a small enum (unlike HTTPStatus) it is very noticeable. |
I think that libraries that don't conform to the new higher standards shouldn't stop the improvement of cpython. |
Also, added documentation |
Thanks for making the requested changes! @corona10: please review the changes made to this pull request. |
You've named the enum HTTPMethod, but currently only represent the REST methods (and CONNECT and TRACE for some reason, even though they're probably even less common than WebDAV). From the RFC:
https://www.iana.org/assignments/http-methods/http-methods.xhtml lists many more registered HTTP methods. Despite the RFC saying methods should be registered there, that's not a requirement, and in the end an HTTP method can be any string.
This isn't a "higher standard", it's just a different value for the same thing. This needs to be very clearly an improvement, not just a change. |
Correct me if I am wrong but WebDAV is an extension to HTTP, not actual part of the standard, CONNECT and TRACE are.
The code is more secure when using an enum, as magic strings are prone to mistypes and therefore errors, |
Convenient maybe, but it requires an import and a longer reference to type, compared to a string. Secure no. Mistypes of http methods result in 405 errors, not security issues. |
Secure is not the right word, I meant less bugs |
I would appreciate your response |
Sorry, I'm not a cpython maintainer. Some of them are at a PyCon sprint right now so I'm sure their notifications are overflowing more than usual. |
@cibofo instead of merging |
@ethanfurman Done |
@cibofo Please create a new PR that addes the other 31 or so registered methods. Thanks! |
@ethanfurman You got it! gonna work on it tomorrow |
python/cpython#91997 `description` isn't actually read-only at runtime, but I don't think there's any other way of telling type checkers "this is an attribute that the members have, not a member itself". And pretending it's a property is already what we do for `HTTPStatus`, which has the same issue.
That is contrary to usual workflow: https://devguide.python.org/pullrequest/ |
python/cpython#91997 `description` isn't actually read-only at runtime, but I don't think there's any other way of telling type checkers "this is an attribute that the members have, not a member itself". And pretending it's a property is already what we do for `HTTPStatus`, which has the same issue.
Huh. Git Bootcamp uses I'll keep using |
#91996