Opened 4 years ago

Closed 4 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)

h.patch (440 bytes) - added by eddie 4 years ago.
modified Request.repr function
6391_improved.patch (2.4 KB) - added by eddie 4 years ago.
added unit test
6391_improve2.patch (2.3 KB) - added by eddie 4 years ago.
I modified the indention.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

Changed 4 years ago by eddie

Attachment: h.patch added

modified Request.repr function

comment:2 Changed 4 years ago by eddie

Keywords: review added

I modified the Request.repr function.

comment:3 Changed 4 years ago by eddie

Keywords: review removed

Changed 4 years ago by eddie

Attachment: 6391_improved.patch added

added unit test

comment:4 Changed 4 years ago by eddie

Keywords: review added

Changed 4 years ago by eddie

Attachment: 6391_improve2.patch added

I modified the indention.

comment:5 Changed 4 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:6 Changed 4 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/http-request-repr-6391

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

comment:7 Changed 4 years ago by Jean-Paul Calderone

(In [37940]) Apple 6391_improve2.patch

refs #6391

comment:8 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Status: assignednew

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.
      }}}
      

comment:9 Changed 4 years ago by Jean-Paul Calderone

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.

Build results

comment:10 Changed 4 years ago by Tom Prince

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:11 Changed 4 years ago by Jean-Paul Calderone

(In [37976]) Vague out the __repr__ docstring

refs #6391

comment:12 Changed 4 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(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.

Note: See TracTickets for help on using tickets.