Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(17)

#21793: httplib client/server status refactor

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 11 months ago by demianbrecht
Modified:
5 years, 3 months ago
Reviewers:
ethan, vadmium+py, berker.peksag, rdmurray, storchaka, victor.stinner
CC:
barry, rhettinger, orsenthil, r.david.murray, stoneleaf, devnull_psf.upfronthosting.co.za, berkerpeksag, Martin Panter, storchaka, demian
Visibility:
Public.

Patch Set 1 #

Total comments: 15

Patch Set 2 #

Total comments: 4

Patch Set 3 #

Total comments: 1

Patch Set 4 #

Total comments: 1

Patch Set 5 #

Total comments: 32

Patch Set 6 #

Patch Set 7 #

Patch Set 8 #

Total comments: 17

Patch Set 9 #

Patch Set 10 #

Total comments: 9

Patch Set 11 #

Patch Set 12 #

Total comments: 3

Patch Set 13 #

Patch Set 14 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/http/server.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
Lib/test/test_httpservers.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 15 chunks +91 lines, -47 lines 0 comments Download

Messages

Total messages: 26
stoneleaf
I noticed some discrepancies with values being in some modules but not in others. Part ...
5 years, 11 months ago #1
demian
http://bugs.python.org/review/21793/diff/12195/Lib/http/__init__.py File Lib/http/__init__.py (right): http://bugs.python.org/review/21793/diff/12195/Lib/http/__init__.py#newcode10 Lib/http/__init__.py:10: On 2014/06/17 23:01:29, stoneleaf wrote: > Instead of making ...
5 years, 11 months ago #2
stoneleaf
Go ahead and upload the new patch. Generally, I think a patch (or set of ...
5 years, 11 months ago #3
stoneleaf
With the exception of the `reason_phrase` attribute, I like it. http://bugs.python.org/review/21793/diff/12199/Doc/library/http.rst File Doc/library/http.rst (right): http://bugs.python.org/review/21793/diff/12199/Doc/library/http.rst#newcode23 ...
5 years, 11 months ago #4
stoneleaf
http://bugs.python.org/review/21793/diff/12222/Doc/library/http.rst File Doc/library/http.rst (right): http://bugs.python.org/review/21793/diff/12222/Doc/library/http.rst#newcode43 Doc/library/http.rst:43: >>> HTTPStatus.OK.reason_phrase Just 'phrase'.
5 years, 11 months ago #5
Martin Panter
https://bugs.python.org/review/21793/diff/12225/Lib/http/__init__.py File Lib/http/__init__.py (right): https://bugs.python.org/review/21793/diff/12225/Lib/http/__init__.py#newcode151 Lib/http/__init__.py:151: VARIANTS_ALSO_NEGOTIATES = 506, 'Variant Also Negotiates' Drop the S ...
5 years, 10 months ago #6
berkerpeksag
Could you also add a release note to Doc/whatsnew/3.5.rst? http://bugs.python.org/review/21793/diff/12431/Doc/library/http.rst File Doc/library/http.rst (right): http://bugs.python.org/review/21793/diff/12431/Doc/library/http.rst#newcode22 Doc/library/http.rst:22: ...
5 years, 5 months ago #7
Martin Panter
http://bugs.python.org/review/21793/diff/12431/Lib/http/server.py File Lib/http/server.py (right): http://bugs.python.org/review/21793/diff/12431/Lib/http/server.py#newcode444 Lib/http/server.py:444: if self.command != 'HEAD' and code >= http.HTTPStatus.OK and ...
5 years, 5 months ago #8
berkerpeksag
On 2014/12/05 10:53:15, vadmium wrote: > http://bugs.python.org/review/21793/diff/12431/Lib/http/server.py > File Lib/http/server.py (right): > > http://bugs.python.org/review/21793/diff/12431/Lib/http/server.py#newcode444 > ...
5 years, 5 months ago #9
demian
http://bugs.python.org/review/21793/diff/12431/Doc/library/http.rst File Doc/library/http.rst (right): http://bugs.python.org/review/21793/diff/12431/Doc/library/http.rst#newcode22 Doc/library/http.rst:22: .. versionchanged:: 3.5 On 2014/12/05 08:10:33, berkerpeksag wrote: > ...
5 years, 5 months ago #10
demian
5 years, 5 months ago #11
r.david.murray
http://bugs.python.org/review/21793/diff/12431/Lib/http/server.py File Lib/http/server.py (right): http://bugs.python.org/review/21793/diff/12431/Lib/http/server.py#newcode444 Lib/http/server.py:444: if self.command != 'HEAD' and code >= http.HTTPStatus.OK and ...
5 years, 5 months ago #12
stoneleaf
http://bugs.python.org/review/21793/diff/12431/Lib/http/__init__.py File Lib/http/__init__.py (right): http://bugs.python.org/review/21793/diff/12431/Lib/http/__init__.py#newcode55 Lib/http/__init__.py:55: 'Object has several resources -- see URI list') Typically, ...
5 years, 5 months ago #13
demian
http://bugs.python.org/review/21793/diff/12431/Lib/http/__init__.py File Lib/http/__init__.py (right): http://bugs.python.org/review/21793/diff/12431/Lib/http/__init__.py#newcode55 Lib/http/__init__.py:55: 'Object has several resources -- see URI list') On ...
5 years, 5 months ago #14
storchaka_gmail.com
Great! Besides two nitpicks the patch LGTM. https://bugs.python.org/review/21793/diff/13395/Lib/http/__init__.py File Lib/http/__init__.py (right): https://bugs.python.org/review/21793/diff/13395/Lib/http/__init__.py#newcode2 Lib/http/__init__.py:2: Add __all__ ...
5 years, 5 months ago #15
victor.stinner_gmail.com
http://bugs.python.org/review/21793/diff/13395/Doc/library/http.rst File Doc/library/http.rst (right): http://bugs.python.org/review/21793/diff/13395/Doc/library/http.rst#newcode28 Doc/library/http.rst:28: reason phrases and long descriptions. You might mention that ...
5 years, 5 months ago #16
stoneleaf
http://bugs.python.org/review/21793/diff/13395/Doc/library/http.rst File Doc/library/http.rst (right): http://bugs.python.org/review/21793/diff/13395/Doc/library/http.rst#newcode47 Doc/library/http.rst:47: Added the *HTTPStatus* Enum Looks like all the status ...
5 years, 5 months ago #17
demian
http://bugs.python.org/review/21793/diff/13395/Doc/library/http.rst File Doc/library/http.rst (right): http://bugs.python.org/review/21793/diff/13395/Doc/library/http.rst#newcode28 Doc/library/http.rst:28: reason phrases and long descriptions. On 2014/12/12 13:09:13, haypo ...
5 years, 5 months ago #18
storchaka_gmail.com
https://bugs.python.org/review/21793/diff/13883/Lib/http/__init__.py File Lib/http/__init__.py (right): https://bugs.python.org/review/21793/diff/13883/Lib/http/__init__.py#newcode28 Lib/http/__init__.py:28: return '{:d} ({:s})'.format(self, self.phrase) This is clever, but I ...
5 years, 3 months ago #19
stoneleaf
https://bugs.python.org/review/21793/diff/13883/Lib/http/__init__.py File Lib/http/__init__.py (right): https://bugs.python.org/review/21793/diff/13883/Lib/http/__init__.py#newcode28 Lib/http/__init__.py:28: return '{:d} ({:s})'.format(self, self.phrase) On 2015/02/09 23:44:17, storchaka wrote: ...
5 years, 3 months ago #20
Martin Panter
https://bugs.python.org/review/21793/diff/13883/Lib/test/test_httpservers.py File Lib/test/test_httpservers.py (right): https://bugs.python.org/review/21793/diff/13883/Lib/test/test_httpservers.py#newcode244 Lib/test/test_httpservers.py:244: self.send_response(HTTPStatus.OK) On 2015/02/09 23:44:17, storchaka wrote: > You can ...
5 years, 3 months ago #21
berkerpeksag
http://bugs.python.org/review/21793/diff/13883/Lib/http/__init__.py File Lib/http/__init__.py (right): http://bugs.python.org/review/21793/diff/13883/Lib/http/__init__.py#newcode28 Lib/http/__init__.py:28: return '{:d} ({:s})'.format(self, self.phrase) On 2015/02/09 23:44:17, storchaka wrote: ...
5 years, 3 months ago #22
storchaka_gmail.com
http://bugs.python.org/review/21793/diff/13883/Lib/http/__init__.py File Lib/http/__init__.py (right): http://bugs.python.org/review/21793/diff/13883/Lib/http/__init__.py#newcode28 Lib/http/__init__.py:28: return '{:d} ({:s})'.format(self, self.phrase) On 2015/02/20 14:23:04, berkerpeksag wrote: ...
5 years, 3 months ago #23
stoneleaf
http://bugs.python.org/review/21793/diff/13981/Lib/http/__init__.py File Lib/http/__init__.py (right): http://bugs.python.org/review/21793/diff/13981/Lib/http/__init__.py#newcode28 Lib/http/__init__.py:28: return int.__str__(self) We should only make this change if ...
5 years, 3 months ago #24
storchaka_gmail.com
http://bugs.python.org/review/21793/diff/13981/Lib/http/__init__.py File Lib/http/__init__.py (right): http://bugs.python.org/review/21793/diff/13981/Lib/http/__init__.py#newcode27 Lib/http/__init__.py:27: def __str__(self): More efficient is HTTPStatus.__str__ = int.__str__ But ...
5 years, 3 months ago #25
demian
5 years, 3 months ago #26
http://bugs.python.org/review/21793/diff/13883/Lib/test/test_httpservers.py
File Lib/test/test_httpservers.py (right):

http://bugs.python.org/review/21793/diff/13883/Lib/test/test_httpservers.py#n...
Lib/test/test_httpservers.py:244: self.send_response(HTTPStatus.OK)
On 2015/02/09 23:44:17, storchaka wrote:
> You can use http.client.OK.

client.* and server.* were only kept for backwards compatibility and I'd
prefer to start using HTTPStatus.* going forward for consistency across server
and client code.

http://bugs.python.org/review/21793/diff/13883/Lib/test/test_httpservers.py#n...
Lib/test/test_httpservers.py:788: LoggingRequestHandlerTestCase,
On 2015/02/09 23:44:17, storchaka wrote:
> Wrong name.

Done.

http://bugs.python.org/review/21793/diff/13981/Lib/http/__init__.py
File Lib/http/__init__.py (right):

http://bugs.python.org/review/21793/diff/13981/Lib/http/__init__.py#newcode28
Lib/http/__init__.py:28: return int.__str__(self)
On 2015/02/20 20:17:34, stoneleaf wrote:
> We should only make this change if our other options are too difficult/too
> complex/not possible.  Changing the __str__ of a stdlib type should be a
> last-resort option.

Seems like the consensus is to make the change in http.client. I'll do that
later today.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+