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

Jump to bottom

Fixed http.client.__all__ and added a test #67627

Closed
vadmium opened this issue Feb 10, 2015 · 11 comments
Closed

Fixed http.client.__all__ and added a test #67627

vadmium opened this issue Feb 10, 2015 · 11 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@vadmium
Copy link
Member

vadmium commented Feb 10, 2015

BPO 23439
Nosy @berkerpeksag, @vadmium, @demianbrecht
Files
  • http.client-all.patch
  • http.client-all.v2.patch
  • http.client-all.v3.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 = None
closed_at = <Date 2015-02-20.07:46:10.849>
created_at = <Date 2015-02-10.22:41:51.425>
labels = ['type-bug', 'library']
title = 'Fixed http.client.__all__ and added a test'
updated_at = <Date 2015-02-20.07:46:10.848>
user = 'https://github.com/vadmium'

    bugs.python.org fields:

    activity = <Date 2015-02-20.07:46:10.848>
actor = 'berker.peksag'
assignee = 'none'
closed = True
closed_date = <Date 2015-02-20.07:46:10.849>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2015-02-10.22:41:51.425>
creator = 'martin.panter'
dependencies = []
files = ['38090', '38140', '38180']
hgrepos = []
issue_num = 23439
keywords = ['patch']
message_count = 11.0
messages = ['235719', '235771', '235772', '235774', '235858', '235868', '236017', '236226', '236250', '236255', '236256']
nosy_count = 4.0
nosy_names = ['python-dev', 'berker.peksag', 'martin.panter', 'demian.brecht']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue23439'
versions = ['Python 3.4', 'Python 3.5']
    @vadmium
    Copy link
    Member Author

    vadmium commented Feb 10, 2015

    This patch was split off my patch for bpo-3566, since it should be less controversial. It adds the HTTPMessage class and the parse_headers() function to __all__.

    I’m not too sure on the status of the parse_headers() function. It is not mentioned in the “http.client” documentation, but is referenced by the “http.server” module’s BaseHTTPRequestHandler.headers entry. Perhaps it should be left unexported?

    @vadmium vadmium added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Feb 10, 2015
    @vadmium
    Copy link
    Member Author

    vadmium commented Feb 11, 2015

    Actually, maybe I should add all those status codes as well, like http.client.OK. Will probably require different patches for 3.4 and 3.5.

    @berkerpeksag
    Copy link
    Member

    It's not that important in my opinion. Let's keep it simple for now :)

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Feb 11, 2015

    The only real question I have is: why? As far as I'm aware, these are
    implementation details of the http.client module (there's even a comment in
    HTTPMessage that it might make sense to move the class altogether).

    As far as the constants go, they're only there for backwards compatibility
    and http.HTTPStatus should be preferred. In light of that and the fact that
    they were not previously in __all__, I agree with Berker about keeping this
    simple for now.

    On Wed, Feb 11, 2015, 13:57 Berker Peksag <report@bugs.python.org> wrote:

    Berker Peksag added the comment:

    It's not that important in my opinion. Let's keep it simple for now :)

    ----------

    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue23439\>

    @vadmium
    Copy link
    Member Author

    vadmium commented Feb 12, 2015

    I don’t have a strong opinion about changing __all__ in these cases. I only noticed the potential problem when I went to add a new class to the module, and thought this was common practice. If we leave it as it is, it would be good to add comment in the source code explaining this decision. Also the test case could still be useful to catch future bugs.

    The currently situation means the status constants are be missing from pydoc help output, and are not available when you do “from http.client import *” in the interactive interpreter.

    HTTPMessage is a strange class indeed; see bpo-5053. But it is referenced a few times by the documentation, so I originally assumed it should be in __all__.

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Feb 13, 2015

    If we leave it as it is, it would be good to add comment in the source code explaining this decision.
    I think that __all__ should be left as-is for the time being. Adding
    some comments around that decision makes sense to me to avoid any future
    confusion around that.

    Also the test case could still be useful to catch future bugs.
    Agreed. I've added a couple minor comments to the review.

    Thanks for the work on this!

    @vadmium
    Copy link
    Member Author

    vadmium commented Feb 15, 2015

    Posting a new patch which explicitly omits HTTPMessage, parse_headers(), and the status codes. Also added and documented the LineTooLong exception. It is already somewhat covered in the test suite.

    See also bpo-21257 about the status of parse_headers().

    @demianbrecht
    Copy link
    Mannequin

    demianbrecht mannequin commented Feb 19, 2015

    Left a super minor comment in Rietveld, but otherwise LGTM.

    @vadmium
    Copy link
    Member Author

    vadmium commented Feb 20, 2015

    Posting v3 patch that changes from a list to a set of expected API names

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 20, 2015

    New changeset 21b31f5438ae by Berker Peksag in branch '3.4':
    Issue bpo-23439: Add missing entries to http.client.__all__.
    https://hg.python.org/cpython/rev/21b31f5438ae

    New changeset 95533c9edaaf by Berker Peksag in branch 'default':
    Issue bpo-23439: Add missing entries to http.client.__all__.
    https://hg.python.org/cpython/rev/95533c9edaaf

    @berkerpeksag
    Copy link
    Member

    Committed now. Thanks for the patch, Martin and thanks for the review, Demian.

    @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
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @berkerpeksag @vadmium