Skip to content
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

feat(API): Support http.HTTPStatus (a proof-of-concept prototype) (#1135) #1334

Closed
wants to merge 6 commits into from

Conversation

vytas7
Copy link
Member

@vytas7 vytas7 commented Jul 2, 2018

This is not a fully polished version, but rather a prototype. Old documentation, tutorials and Python docstrings are still largely intact. It would be awesome to first discuss the proposed changeset with @kgriffs and @jmvrbanac, and distill an acceptable technical solution first; I would be happy to contribute to the documentation afterwards.

The main design ideas laid out below are roughly in-line with the plan found in #1135 :

  • Response.status shall be set to http.HTTPStatus, or its backport to older Python versions found in falcon.util.status_enum
  • Integer status codes implicitly work too (both a convenience and logical inheritance wise: http.HTTPStatus subclasses an int via IntEnum)
  • In the same falcon.util.status_enum, a base HTTP status enum class is defined similarly to http.HTTPStatus; that is used to support other non-standard status codes previously included in Falcon [1]
  • Falcon usual status code constants are aliased to the new enum values [2], so a fair share of users could hopefully continue using most of their code as-is

Somewhat controversial ramifications:

  • It may be cumbersome to define arbitrary codes; with the proposed changeset, one would need to resort to manually adding the string status to falcon.status_codes.COMBINED_STATUS_CODE lookup table. This could be made easier by either implementing support for legacy raw status, or probably by moving some part of this machinery to Response class, and documenting how and what to override to cram arbitrary status in; most probably both alternatives would come at a performance cost.

Other issues:

  • Some of https://github.com/timothycrosley/hug tests are failing because of the same reason many Falcon tests were failing (assuming that falcon.HTTP_XXX are raw string statuses and using those values to check response status). @timothycrosley might now more, but from what I have seen in hug source code, it uses falcon.HTTP_XXX constants in a clean fashion, so it is just a problem with a couple of unit tests, not Hug API functionality.
  1. As advertised!

What other framework has integrated support for 786 TRY IT NOW?

  1. On Python 3:
>>> import falcon
>>> import http
>>> falcon.HTTP_404
<HTTPStatus.NOT_FOUND: 404>
>>> falcon.HTTP_404.phrase
'Not Found'
>>> int(falcon.HTTP_404)
404
>>> falcon.HTTP_FORBIDDEN
<HTTPStatus.FORBIDDEN: 403>
>>> falcon.HTTP_FORBIDDEN is http.HTTPStatus(403)
True

@@ -803,6 +804,10 @@ def add_link(self, target, rel, title=None, title_star=None,

""")

# @property
Copy link
Member Author

Choose a reason for hiding this comment

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

An open question: could something like this be of use to API users?

'supported status code' % status_code)


def get_http_status_line(status):
Copy link
Member Author

@vytas7 vytas7 Jul 2, 2018

Choose a reason for hiding this comment

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

Suggestions for a better name are very welcome as this is a bit of a misnomer.

Technically HTTP status line includes HTTP protocol, so a full status would look like HTTP/1.1 404 Not Found.

Edit: maybe get_raw_http_status would look better?

@jgirardet
Copy link

Raw is better

@vytas7
Copy link
Member Author

vytas7 commented Nov 3, 2018

Killing this for now, I will try to port optimizations and design changes in small increments instead.

@vytas7 vytas7 closed this Nov 3, 2018
@vytas7 vytas7 deleted the support-http.HTTPStatus branch August 20, 2024 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants