| msg220860 - (view) |
Author: Demian Brecht (demian.brecht) *  |
Date: 2014-06-17 19:02 |
| This patch is a follow up to an out of scope comment made by R. David Murray in #20898 (http://bugs.python.org/issue20898#msg213771). In a nutshell, there is some redundancy between http.client and http.server insofar as the definition of http status code, names and descriptions go. The included patch is a stab at cleaning some of this up while remaining backwards compatible and is intended to solicit feedback before finishing work. TODOs: * Populate descriptions for status codes * Documentation * Tests (?) |
|
|
| msg220890 - (view) |
Author: Ethan Furman (ethan.furman) *  |
Date: 2014-06-17 21:02 |
| Left comments in reitvald about modifying the Enum used -- let me know if you have any questions about that. |
|
|
| msg220924 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-06-18 07:07 |
| Attached new patch with changes from review and a stab at an update to the docs. |
|
|
| msg221097 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-06-20 15:18 |
| New patch attached addressing review comments. |
|
|
| msg221110 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-06-20 18:48 |
| Updated patch with silly missed doc update. |
|
|
| msg223088 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-07-15 05:30 |
| Bump for a follow-up review or merge |
|
|
| msg223603 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-07-21 19:48 |
| Removed draft status code, removed S from VARIANTS_ |
|
|
| msg232171 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-12-05 01:21 |
| Bump (again) for hopeful merge. |
|
|
| msg232176 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2014-12-05 07:11 |
| I left a couple of comments on Rietveld. Thanks for the patch, Demian. |
|
|
| msg232198 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-12-05 16:38 |
| Updated patch addressing review comments. Thanks for the review. |
|
|
| msg232509 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-12-12 05:42 |
| Self review/update: removed two errant breakpoints and updated the what's new section (missed in my previous patch). |
|
|
| msg232588 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2014-12-13 02:41 |
| Updated patch addressing further reviews |
|
|
| msg233046 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2014-12-23 14:30 |
| New changeset edf669b13482 by Serhiy Storchaka in branch 'default': Issue #21793: Added http.HTTPStatus enums (i.e. HTTPStatus.OK, https://hg.python.org/cpython/rev/edf669b13482 |
|
|
| msg233047 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-12-23 14:31 |
| Thank you for your contribution Demian. |
|
|
| msg235547 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-02-08 05:07 |
| Currently the log output includes the new HTTPStatus codes. I don’t care much for the log output, but perhaps this wasn’t part of the plan? Before: $ python3.4 -m http.server Serving HTTP on 0.0.0.0 port 8000 ... 127.0.0.1 - - [08/Feb/2015 05:05:28] "GET / HTTP/1.1" 200 - After: $ ./python -m http.server Serving HTTP on 0.0.0.0 port 8000 ... 127.0.0.1 - - [08/Feb/2015 05:05:40] "GET / HTTP/1.1" HTTPStatus.OK - |
|
|
| msg235560 - (view) |
Author: Ethan Furman (ethan.furman) * |
Date: 2015-02-08 17:40 |
| Without having looked at the code I would guess the fix is as simple as changing a %s to a %d. Having said that, it would be nice if the name was also in the log -- something like: 127.0.0.1 - - [08/Feb/2015 05:05:28] "GET / HTTP/1.1" 200 (OK) - |
|
|
| msg235638 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2015-02-09 22:01 |
| Thanks for the catch Martin, it definitely wasn't intended. I've added a patch to fix the issue and add the extra description as suggested by Ethan. |
|
|
| msg235651 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-02-10 00:26 |
| If changing the log format, you might also want to update the comment at the top of Lib/http/server.py:53. It seems the original format was imitating <https://en.wikipedia.org/wiki/Common_Log_Format>, except the timestamp is slightly different. |
|
|
| msg235690 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2015-02-10 15:19 |
| I’ve reverted the patch to use the old format. The main reason being that plain ints can still be used in most cases as values for code, in which case logging will be inconsistent with cases using the enum. |
|
|
| msg235712 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-02-10 21:54 |
| New logfix patch looks good. I would have written format(self) or format(self, 'd') instead of '{:d}'.format(self), but that’s no big deal. |
|
|
| msg235738 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-02-11 11:47 |
| FYI I opened Issue 23442 for a separate regression to do with the REQUEST_HEADER_FIELDS_TOO_LARGE name |
|
|
| msg236281 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-02-20 13:24 |
| LGTM. I left a comment for Serhiy on Rietveld. |
|
|
| msg236284 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-02-20 13:49 |
| Does not changing __str__ to decimal representation (and in this case __str__ = int.__str__ may be better) lost a part of the point of converting HTTP status codes to enums? |
|
|
| msg236304 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2015-02-20 16:31 |
| The updated patch addresses comments which I’d somehow missed previously, but keeps the log fix to the __str__ implementation of HTTPStatus (using int.__str__ rather than format()). > Does not changing __str__ to decimal representation (and in this case __str__ = int.__str__ may be better) lost a part of the point of converting HTTP status codes to enums? I don’t think so. In the case of HTTPStatus in general, I think that the optimal string representation of an element of the enum is the stringified version of the status code. If nothing else, it’s consistent with the other type of status code that can be used (ints). That does lead me to something that I think is a little odd about IntEnums in general but I’ll ask that question in python-dev rather than here as to not conflate this issue. |
|
|
| msg236340 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-02-20 22:39 |
| One option might be changing log_request() from self.log_message('"%s" %s %s', self.requestline, str(code), str(size)) to self.log_message('"%s" %s %s', self.requestline, format(code), size) Using str() is redundant with %s, and using format() instead invokes the int base class’s __format__() rather than the enum’s __repr__(). |
|
|
| msg236341 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-02-20 22:50 |
| I would write just: if isinstance(code, HTTPStatus): code = '%d' % code |
|
|
| msg236342 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2015-02-20 22:56 |
| > On Feb 20, 2015, at 2:50 PM, Serhiy Storchaka <report@bugs.python.org> wrote: > > if isinstance(code, HTTPStatus): > code = '%d' % code That’s what I’m intending on doing. It’s definitely not as contained as changing the __str__ implementation of HTTPStatus. That said, I understand the reasoning behind not doing so and this path is the next clearest. |
|
|
| msg236347 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2015-02-21 01:40 |
| Latest patch should address all comments. It also fixes the same issue in error logging which wasn’t previously accounted for. The test file has also been updated with using HTTPStatus where possible rather than hard coded ints. This is consistent with other areas in the http package. |
|
|
| msg236349 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2015-02-21 04:11 |
| I think you will find the error logging was already fine, since it already uses %d: 127.0.0.1 - - [21/Feb/2015 04:02:06] code 404, message File not found 127.0.0.1 - - [21/Feb/2015 04:02:06] "GET /nonexistant HTTP/1.1" HTTPStatus.NOT_FOUND - The new codes in the tests look okay though. |
|
|
| msg236370 - (view) |
Author: Demian Brecht (demian.brecht) * |
Date: 2015-02-21 16:03 |
| Well, I’m not entirely sure how I came to the conclusion that errors were a problem (other than not spending enough time walking through it) and of course you’re right about the coercion handling it just fine. I’ve removed the explicit conversion in the code but have left the tests as they’re not tested elsewhere. Hopefully that should now wrap this up. |
|
|
| msg237431 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-03-07 09:52 |
| New changeset ad64b6a7c0e2 by Serhiy Storchaka in branch 'default': Issue #21793: BaseHTTPRequestHandler again logs response code as numeric, https://hg.python.org/cpython/rev/ad64b6a7c0e2 |
|
|
| msg237432 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-03-07 09:55 |
| Thank you Martin for noticing a logging issue. |
|
|