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

Missing 507 response description #65097

Closed
FilipMalczak mannequin opened this issue Mar 12, 2014 · 22 comments
Closed

Missing 507 response description #65097

FilipMalczak mannequin opened this issue Mar 12, 2014 · 22 comments
Assignees
Labels
docs Documentation in the Doc dir easy type-feature A feature request or enhancement

Comments

@FilipMalczak
Copy link
Mannequin

FilipMalczak mannequin commented Mar 12, 2014

BPO 20898
Nosy @rhettinger, @orsenthil, @bitdancer, @florentx, @berkerpeksag, @vadmium, @demianbrecht, @dangro
Files
  • issue20898_with_doc.patch
  • issue20898.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/berkerpeksag'
    closed_at = <Date 2015-01-20.04:33:13.268>
    created_at = <Date 2014-03-12.15:14:04.221>
    labels = ['easy', 'type-feature', 'docs']
    title = 'Missing 507 response description'
    updated_at = <Date 2015-11-20.07:16:26.701>
    user = 'https://bugs.python.org/FilipMalczak'

    bugs.python.org fields:

    activity = <Date 2015-11-20.07:16:26.701>
    actor = 'SilentGhost'
    assignee = 'berker.peksag'
    closed = True
    closed_date = <Date 2015-01-20.04:33:13.268>
    closer = 'berker.peksag'
    components = ['Documentation']
    creation = <Date 2014-03-12.15:14:04.221>
    creator = 'Filip.Malczak'
    dependencies = []
    files = ['34450', '37604']
    hgrepos = []
    issue_num = 20898
    keywords = ['patch', 'easy']
    message_count = 22.0
    messages = ['213266', '213268', '213654', '213771', '213781', '213786', '220797', '223617', '233230', '233234', '233273', '233465', '233466', '234095', '234106', '234339', '234340', '234341', '234349', '234351', '234352', '254952']
    nosy_count = 11.0
    nosy_names = ['rhettinger', 'orsenthil', 'r.david.murray', 'SilentGhost', 'flox', 'python-dev', 'berker.peksag', 'martin.panter', 'demian.brecht', 'Filip.Malczak', 'Daniel.Andrade.Groppe']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue20898'
    versions = ['Python 3.5']

    @FilipMalczak
    Copy link
    Mannequin Author

    FilipMalczak mannequin commented Mar 12, 2014

    I find it strange, that in http.client module we have variable:
    INSUFFICIENT_STORAGE = 507
    yet in responses (dict mapping int codes to descriptions) 507 is missing.

    It's probably just mistake caused by short dev memory, fix is easy: add line:
    507: 'Insufficient storage',
    between lines 208 and 209 (just after mapping for 505).

    Sorry, if this isn't well formatted issue, or if I specified wrong metadata.

    I'm working on Python 3.3.2+ (automatically installed in LUbuntu 13.10), and I don't know whether it was fixed in later versions.

    @FilipMalczak FilipMalczak mannequin added topic-IO type-bug An unexpected behavior, bug, or error labels Mar 12, 2014
    @FilipMalczak FilipMalczak mannequin changed the title Missin 507 response description Missing 507 response description Mar 12, 2014
    @bitdancer
    Copy link
    Member

    Looks like that's not the only code that was missed (I see 102 is also not listed).

    What happened was that the responses table was originally part of urllib2, and was moved into httplib (back when it was httplib). But httplib had more response codes in it than the urllib2 did, apparently.

    I think it makes sense to make them consistent (add the missing ones to responses), even though it is not clear that the constants are used anywhere.

    I wonder if it makes sense to use an Enum here somehow...

    @benjaminp benjaminp added easy stdlib Python modules in the Lib dir and removed topic-IO labels Mar 12, 2014
    @dangro
    Copy link
    Mannequin

    dangro mannequin commented Mar 15, 2014

    Added missing constants and response status codes:
    Constants:
    ALREADY_REPORTED, PERMANENT_REDIRECT, LOOP_DETECTED

    Response status codes:
    102, 207, 208, 226, 308, 422, 423, 424, 426, 507, 508

    @dangro dangro mannequin added extension-modules C modules in the Modules dir type-feature A feature request or enhancement and removed stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 15, 2014
    @bitdancer
    Copy link
    Member

    Thanks. That patch looks good except that it is missing the corresponding documentation changes, but...

    I just noticed that this table is a partial duplicate of the table in http.server. I suspect this has historical origins, but I don't see any reason to maintain the duplication. I wonder if we should do some refactoring here. This is getting a beyond the original scope of this issue, but it seems to me that the common stuff used by http.server and http.client (which is mostly in http.client, except for the 'explain' messages that are in the http.server responses list) could be moved into a 'common' or 'util' module, and imported from both, thus reducing the direct coupling between http.server and http.client. (The 'responses' object in http.client would have to be computed from the one in this new module, since the http.client one doesn't contain the 'explain' messages).

    @FilipMalczak
    Copy link
    Mannequin Author

    FilipMalczak mannequin commented Mar 16, 2014

    If we're getting out of original scope, then I wonder... Maybe we should keep only standard status codes here? If not, which should we support, and which not? What about custom Spring 420 Method Failure?

    One way to clean up mess here is to create some dictionaries called Http09, Http10, Http11, but also Nginx, Spring, InternetDraft, etc - they would hold codes specific to some standard or extension (and they should be incremental, so Http11 would hold only those codes that came with HTTP 1.1, not any of HTTP 1.0 codes).

    Also, we should provide function responses_mapping which should take dictionaries with code mappings, so one may use it like responses_mapping(Http09, Http10, Http11, Spring). Module attribute responses should be initialized to responses_mapping(Http09, Http10, Http11), so it would contain standard codes only.

    responses_mapping (we should probably consider different name) should take any number of dicts, merge them and return merging result. Additionally, I think it should overwrite merged values, so if we call it with two dicts, which hold the same key - value from last dict with this key should be used. Essentially, it is just:
    def responses_mapping(*dicts):
    out = {}
    for d in dicts:
    out.update(d)
    return out

    Now, we would have clean responses dictionary (mentioned in first comment, one that started this issue), and possibility to extend responses set with different protocol and extension versions.

    There may be one problem - many apps and libs (possibly standard python library too) may use keys from out of HTTP standard, so this change could break backwards compability.

    @dangro
    Copy link
    Mannequin

    dangro mannequin commented Mar 17, 2014

    Here goes the patch once again, this time with the changes to the documentation.

    Two files were modified:

    1. /Lib/http/client.py
    2. /Doc/library/http.client.rst

    Hope this helps until a decision is made on how to remove duplicate codes in http.clinet and http.server

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Jun 17, 2014

    I actually made a similar change for issue bpo-15025 earlier today. One of the two should likely be closed as a dupe.

    @rhettinger rhettinger self-assigned this Jun 21, 2014
    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Jul 21, 2014

    Being this is tagged for 3.5, I've refactored status codes as part of bpo-21793. Should that be accepted and merged, that will also clear up this issue.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Dec 31, 2014

    The patch in bpo-21793 has been merged, resolving this issue as well. This should now be closed.

    @berkerpeksag
    Copy link
    Member

    I think the documentation part of the patch is still useful.

    @berkerpeksag berkerpeksag added docs Documentation in the Doc dir and removed extension-modules C modules in the Modules dir labels Dec 31, 2014
    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Jan 1, 2015

    @berker: Good point, although I think that the status code table in http.client.rst should be merged with the one in http.rst as to avoid redundancy (newly added status codes should also have links added). The table in http.client.rst should likely be replaced with a link to the newer one.

    @rhettinger rhettinger removed their assignment Jan 4, 2015
    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Jan 5, 2015

    The attached patch is a rework of the http.HTTPStatus docs to include links to the RFCs. While working through this, I noticed that I may have been a little overzealous in inclusion of some of the status codes. Some non-standard codes have been deprecated or collide between vendors. As such, I've removed non-standard codes. The only ones supported are those that are IANA-registered (including experimental codes). This is to reduce the chance of future conflicts. These were only recently added for 3.5 and therefore should not cause any backwards compatibility issues.

    @berkerpeksag
    Copy link
    Member

    LGTM. Great patch, thanks!

    @berkerpeksag berkerpeksag self-assigned this Jan 5, 2015
    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Jan 15, 2015

    Ping. Would be nice to get this change in before 3.5.0a1.

    @berkerpeksag
    Copy link
    Member

    This is mostly a documentation update. Documentation updates can be committed anytime. Also, feature freeze for 3.5 will be started by Beta 1, not Alpha 1 (see PEP-478).

    I'll commit the patch this weekend. Thanks!

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 20, 2015

    New changeset c8647dab4780 by Berker Peksag in branch 'default':
    Issue bpo-20898: Add a "HTTP status codes" section to avoid duplication in HTTP docs.
    https://hg.python.org/cpython/rev/c8647dab4780

    @berkerpeksag
    Copy link
    Member

    Committed now, sorry about the delay. Thanks for the patch, Demian.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Jan 20, 2015

    No worries, thanks for taking care of merging it Berker.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 20, 2015

    Just noticed the new documentation says “http.HTTPStatus.OK is also available as . . . http.server.OK”. I think this is wrong; only the client module (and now the top-level package) have those constants. The enum values are only available in the server module via http.server.BaseHTTPRequestHandler.responses.keys() as far as I can tell.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 20, 2015

    New changeset 3a95a74aca4e by Berker Peksag in branch 'default':
    Issue bpo-20898: Enum names are only available in the http.client module as constants.
    https://hg.python.org/cpython/rev/3a95a74aca4e

    @berkerpeksag
    Copy link
    Member

    Good catch, thank you Martin.

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Nov 20, 2015

    Re msg233465: it doesn't seem like a particularly good justification to remove something that is not hurting anyone. The problem now is that because http.HTTPStatus is an enumeration, it cannot be extended, therefore when someone encounters these non-standard codes, the only option is to just use the integers. I trust what Demian is saying, that there might be some clashing between the non-standard codes, but until it's shown to be a problem removing real-world use cases just actively reduces usefulness of the library. For comparison, the module that is actually used by people includes the whole lot https://github.com/kennethreitz/requests/blob/master/requests/status_codes.py

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    docs Documentation in the Doc dir easy type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants