Improve the format of http.Request.__repr__ to include eg class and object identity

[Richard Wall]

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.

• ​http://stackoverflow.com/questions/452300/python-object-repr-self-should-be-an-expression

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

[eddie]

modified Request.repr function

[eddie]

I modified the Request.repr function.

[eddie]

added unit test

[eddie]

I modified the indention.

[Jean-Paul Calderone]

(In [37939]) Branching to 'http-request-repr-6391'

[Jean-Paul Calderone]

(In [37940]) Apple 6391_improve2.patch

refs #6391

[Jean-Paul Calderone]

Thanks! A couple minor points:

1. in twisted/web/test/test_http.py
   1. Lines should be wrapped at column 79.
   2. instead of C{string}, you should write L{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 want str instead.
   1. 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.
   2. 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.
      }}}
      

[Jean-Paul Calderone]

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.

​Build results

[Tom Prince]

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.

[Jean-Paul Calderone]

(In [37976]) Vague out the __repr__ docstring

refs #6391

[Jean-Paul Calderone]

(In [37977]) Merge http-request-repr-6391

Author: introom, exarkun Reviewer: exarkun, tom.prince Fixes: #6391, #6120

Expand the output of twisted.web.http.Request.__repr__ to include a bit more information. Also add unit tests for it.