classification
Title: Fixed http.client.__all__ and added a test
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, demian.brecht, martin.panter, python-dev
Priority: normal Keywords: patch

Created on 2015-02-10 22:41 by martin.panter, last changed 2015-02-20 07:46 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
http.client-all.patch martin.panter, 2015-02-10 22:42 review
http.client-all.v2.patch martin.panter, 2015-02-15 02:33 review
http.client-all.v3.patch martin.panter, 2015-02-20 04:14 review
Messages (11)
msg235719 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-10 22:41
This patch was split off my patch for Issue 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?
msg235771 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-11 21:49
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.
msg235772 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-02-11 21:57
It's not that important in my opinion. Let's keep it simple for now :)
msg235774 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-11 22:30
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>
> _______________________________________
>
msg235858 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-12 22:09
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 Issue 5053. But it is referenced a few times by the documentation, so I originally assumed it should be in __all__.
msg235868 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-13 01:13
> 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!
msg236017 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-15 02:33
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 Issue 21257 about the status of parse_headers().
msg236226 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-02-19 17:11
Left a super minor comment in Rietveld, but otherwise LGTM.
msg236250 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-20 04:14
Posting v3 patch that changes from a list to a set of expected API names
msg236255 - (view) Author: Roundup Robot (python-dev) Date: 2015-02-20 07:44
New changeset 21b31f5438ae by Berker Peksag in branch '3.4':
Issue #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 #23439: Add missing entries to http.client.__all__.
https://hg.python.org/cpython/rev/95533c9edaaf
msg236256 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-02-20 07:46
Committed now. Thanks for the patch, Martin and thanks for the review, Demian.
History
Date User Action Args
2015-02-20 07:46:10berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg236256

stage: patch review -> resolved
2015-02-20 07:44:19python-devsetnosy: + python-dev
messages: + msg236255
2015-02-20 04:14:23martin.pantersetfiles: + http.client-all.v3.patch

messages: + msg236250
2015-02-19 17:11:13demian.brechtsetmessages: + msg236226
2015-02-15 02:33:24martin.pantersetfiles: + http.client-all.v2.patch

messages: + msg236017
2015-02-13 01:13:27demian.brechtsetmessages: + msg235868
2015-02-12 22:09:02martin.pantersetmessages: + msg235858
2015-02-11 22:30:04demian.brechtsetmessages: + msg235774
2015-02-11 21:57:42berker.peksagsetmessages: + msg235772
2015-02-11 21:49:40martin.pantersetmessages: + msg235771
2015-02-11 21:24:52demian.brechtsetnosy: + demian.brecht
2015-02-11 13:21:43berker.peksagsetnosy: + berker.peksag
stage: patch review

versions: + Python 3.4, Python 3.5
2015-02-10 22:42:04martin.pantersetfiles: + http.client-all.patch
keywords: + patch
2015-02-10 22:41:51martin.pantercreate