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

#26586: Simple enhancement to BaseHTTPRequestHandler

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 2 months ago by angwerzx
Modified:
4 years, 2 months ago
Reviewers:
vadmium+py
CC:
devnull_psf.upfronthosting.co.za, Martin Panter, xiang.zhang
Visibility:
Public.

Patch Set 1 #

Total comments: 5

Patch Set 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/http/server.py View 1 5 chunks +20 lines, -15 lines 0 comments Download
Lib/test/test_httpservers.py View 1 1 chunk +8 lines, -1 line 0 comments Download

Messages

Total messages: 1
Martin Panter
4 years, 2 months ago #1
https://bugs.python.org/review/26586/diff/16768/Lib/http/server.py
File Lib/http/server.py (right):

https://bugs.python.org/review/26586/diff/16768/Lib/http/server.py#newcode340
Lib/http/server.py:340: HTTPStatus.BAD_REQUEST,
BAD_REQUEST is the generic 400 code, so it might be an improvement to use the
more specific REQUEST_HEADER_FIELDS_TOO_LARGE (431) code
(https://tools.ietf.org/html/rfc6585).

https://bugs.python.org/review/26586/diff/16768/Lib/http/server.py#newcode341
Lib/http/server.py:341: "Too many headers"
Perhaps we can use the actual message from the exception (got more than %d
headers)

https://bugs.python.org/review/26586/diff/16768/Lib/http/server.py#newcode490
Lib/http/server.py:490: ("%s %d %s\r\n" % (self.protocol_version, code,
message))
What changed here? IMO the original was more readable. Anyway, I would prefer to
see it as two or three statements on separate lines.

Please do not change style stuff unless the new version is clearly superior, or
you add a comment explaining why it should not be changed again. It just adds
noise to the code review and history, and the next person will just come along
and change it again to their own personal style.

https://bugs.python.org/review/26586/diff/16768/Lib/test/test_httpservers.py
File Lib/test/test_httpservers.py (right):

https://bugs.python.org/review/26586/diff/16768/Lib/test/test_httpservers.py#...
Lib/test/test_httpservers.py:863: headers = ['header%d: test\r\n' for i in
range(101)]
Did you mean to add ā€œ% iā€ in there? You could use the new bytes formatting b'. .
.' % i. Or it might be simpler to just write b'header: test\r\n' * 101.

https://bugs.python.org/review/26586/diff/16768/Lib/test/test_httpservers.py#...
Lib/test/test_httpservers.py:865: b'GET / HTTP/1.1\r\n' +
''.join(headers).encode('utf-8') + b'\r\n')
I would encode with ASCII. There should only be ASCII characters in HTTP fields.
Sign in to reply to this message.

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