Opened 9 years ago
Closed 9 years ago
#6391 enhancement closed fixed (fixed)
Improve the format of http.Request.__repr__ to include eg class and object identity
Reported by: | Richard Wall | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | web | Keywords: | |
Cc: | jknight | Branch: |
branches/http-request-repr-6391
branch-diff, diff-cov, branch-cov, buildbot |
Author: | exarkun |
Description
In #6120 radix commented that Request.repr should include the ClassName and memory address.
19:24 < radix> rwall: hmm 19:25 < radix> rwall: can you please include the class name and instance ID in that repr as well 19:25 < radix> I really hate it when classes __repr__ don't include the basic info 19:26 < radix> if you want me to put a comment on the ticket I can do that 19:26 < radix> oh, you're reviewing, not authoring. 19:26 < rwall> radix: Yes. That sounds like a good idea. Is it ok to change the repr without warning? 19:27 < radix> I see. and it's not even changing the contents 19:27 < rwall> Yep, just adding a missing test. 19:27 < radix> rwall: yes, as tomprince said in an earlier comment on the ticket, we don't want users relying on the content of the __repr__ 19:27 < radix> but I see the content isn't even being changed in this branch, so it's probably not the appropriate branch to change it in. 19:29 < rwall> radix: Ok, but I'll raise another ticket. What's the ideal repr? What should be the standard? 19:30 < rwall> radix: and without wanting to dwell too long on such a small ticket...do you think __repr__ should have full docstring? 19:32 < radix> huh, standards, that's an idea. we don't have any 19:32 < radix> rwall: I don't think __repr__'s docstring needs to document the format absolutely. but I don't really think it matters either way. 19:33 < rwall> ok 19:34 < radix> Personally, off the cuff, I would suggest something like <ClassName at 0xffffffff ...> or something, where "..." could be "GET /" or maybe even "method=GET path=/" etc
Here's an interesting discussion about ideal repr output.
If at all possible, this (repr) should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment).
If this is not possible, a string of the form <...some useful description...> should be returned.
The return value must be a string object. If a class defines repr() but not str(), then repr() is also used when an “informal” string representation of instances of that class is required.
This is typically used for debugging, so it is important that the representation is information-rich and unambiguous
The current repr outputs method='GET' etc but in #6120, exarkun commented that he'd rather just see method=GET. I think his reasoning was that ultimately, the method and clientproto should be supplied and stored as twisted.python.constants instead of byte strings. And similarly, the URI should also probably be supplied and stored as a URI object of some sort. This would enforce the use of byte strings for these attribute values. (see related ticket:6168).
So here's a proposed format...
<twisted.web.http.Request at 0xffffffff method=GET uri=http://www.example.com/foo/bar clientproto=HTTP/1.1>
See:
- source:trunk/twisted/web/http.py
Attachments (3)
Change History (15)
comment:1 Changed 9 years ago by
Cc: | jknight added |
---|
Changed 9 years ago by
comment:3 Changed 9 years ago by
Keywords: | review removed |
---|
comment:4 Changed 9 years ago by
Keywords: | review added |
---|
comment:5 Changed 9 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:6 Changed 9 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/http-request-repr-6391 |
(In [37939]) Branching to 'http-request-repr-6391'
comment:8 Changed 9 years ago by
Keywords: | review removed |
---|---|
Status: | assigned → new |
Thanks! A couple minor points:
- in twisted/web/test/test_http.py
- Lines should be wrapped at column 79.
- instead of
C{string}
, you should writeL{str}
.
- L is for "links"... sort of. Any time you have a name for an object that probably has documentation somewhere else, L makes sense. If you just have some stuff that is more or less Python code (for example, "a + b"), C is appropriate.
str
is the name of a builtin type, string is either the name of a stdlib module (which doesn't seem to be related to the docstring you're writing) or just a plain old english word. Either way, you wantstr
instead.
- The unit test should really be two unit tests, one for each form the repr result can take. A good rule of thumb is one assertion per test.
- As a matter of style (so you can take this part of the review or leave it), the indentation convention where you have either:
assertEqual(foo, bar) }}} or {{{ assertEqual( foo, bar) }}} seems preferable to the one where you have: {{{ assertEqual(foo, bar) }}} 1. twisted/web/http.py 1. in the docstring for `__repr__`, it doesn't seem necessary to duplicate the explanation of the format of the result. 1. Same string/str, C/L comment as above 1. I slightly disagree with the ticket description's request for "twisted.web.http.Request" to appear in the repr output, instead of just "Request". The former is quite a mouthful, and you probably already know you're dealing with the web once you see the rest of the string. 1. There's one extra feature not being tested here, which is that a subclass of a different name will get a slightly different repr result (reflecting the subclass name instead of just "Request"). 1. Needs a topfile, probably a misc. }}}
comment:9 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
Well, oops, forgot to preview that comment and spend 45 minutes fixing the markup. Sorry about that. Anyhow, I went ahead and addressed those points myself - r37941.
comment:10 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
I don't think including the exact format of __repr__
in warranted. Users shouldn't be depending on the format. Please merge, after deleting that bit from the docstring.
Note: This supersedes #6120, so that ticket should be closed with this one, as well.
comment:12 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
modified Request.repr function