Opened 11 years ago

Closed 11 years ago

#3695 defect closed fixed (fixed)

repr of twisted.protocols.amp.AMP is confusing

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: core Keywords: amp, repr, easy
Cc: Branch: branches/amp-repr-3695
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

Someone asked why they have None in the repr of one of their AMP instances: <AuctionProtocol None at 0x35a9a50>. None here is the inner protocol, a feature of AMP which many people will probably never use. It would be better to label this value, and perhaps to omit it when there is no inner protocol.

Attachments (10)

amp.patch (620 bytes) - added by cary 11 years ago.
3695_unittests.patch (1005 bytes) - added by SeanMcQ 11 years ago.
Unit tests for patch 3695.
3695_unittests2.patch (1.0 KB) - added by SeanMcQ 11 years ago.
Unit tests for patch 3695. Actually testing reported defect.
3695_complete.patch (1.8 KB) - added by SeanMcQ 11 years ago.
Updated unit tests as per review. Updated amp.py patch to use if statement for clarity and to use naming standards.
3695_complete.2.patch (1.8 KB) - added by SeanMcQ 11 years ago.
3695_complete.3.patch (1.8 KB) - added by SeanMcQ 11 years ago.
3695_complete.4.patch (1.8 KB) - added by SeanMcQ 11 years ago.
3695_complete.5.patch (2.4 KB) - added by SeanMcQ 11 years ago.
3695_complete.6.patch (2.3 KB) - added by SeanMcQ 11 years ago.
3695_complete.7.patch (2.9 KB) - added by SeanMcQ 11 years ago.

Download all attachments as: .zip

Change History (31)

Changed 11 years ago by cary

Attachment: amp.patch added

comment:1 Changed 11 years ago by cary

Keywords: amp repr review added
Owner: Glyph deleted

If innerProtocol is None: <Foo at 0xb8e950>

If innerProtocol is set: <Foo inner<Bar at 0xb8e8f0> at 0xb8e950>

comment:2 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to cary

New repr looks good to me. It needs unit tests, though. Also, inner_proto should be innerProto.

Changed 11 years ago by SeanMcQ

Attachment: 3695_unittests.patch added

Unit tests for patch 3695.

Changed 11 years ago by SeanMcQ

Attachment: 3695_unittests2.patch added

Unit tests for patch 3695. Actually testing reported defect.

comment:3 Changed 11 years ago by SeanMcQ

Keywords: review added

comment:4 Changed 11 years ago by David Reid

Keywords: review removed
Owner: changed from cary to SeanMcQ

Ok, your tests should use assert_, there is an assertIn which would be better for this.

Also the repr implementation would probably be clearer if you just used an if statement.

and inner_repr should be innerRepr (see the twisted coding standard for variable naming conventions)

the % operator should always be passed a tuple or a dict.

so it should be ' inner%s' % (self.innerProtocol,) Also you might want a space between inner and the %s.

Changed 11 years ago by SeanMcQ

Attachment: 3695_complete.patch added

Updated unit tests as per review. Updated amp.py patch to use if statement for clarity and to use naming standards.

comment:5 Changed 11 years ago by SeanMcQ

Keywords: review added

Changed 11 years ago by SeanMcQ

Attachment: 3695_complete.2.patch added

comment:6 Changed 11 years ago by SeanMcQ

Regression in previous patch. Output was of the format <AMPinner...> when it was expected to be <AMP inner...>. 3695_complete.2.patch updates to correct this error.

comment:7 Changed 11 years ago by David Reid

Keywords: review removed

Ok, there is now trailing whitespace in your test methods and the docstring for test_innerProtocolNotInRepr should be wrapped after "innerProtocol".

The string formatting should probably use %r so you definitely get the repr result of the object and not the str.

Also there is trailing whitespace on the return line of AMP.repr

Changed 11 years ago by SeanMcQ

Attachment: 3695_complete.3.patch added

Changed 11 years ago by SeanMcQ

Attachment: 3695_complete.4.patch added

comment:8 Changed 11 years ago by SeanMcQ

Keywords: review added

Updated per review.

comment:9 Changed 11 years ago by SeanMcQ

Owner: SeanMcQ deleted

comment:10 Changed 11 years ago by David Reid

Keywords: review removed
Owner: set to SeanMcQ

Do complete string comparison to check for the repr.

You can do

self.assertEquals(repr(a), "<AMP inner <TestProto None 0x%x> 0x%x>" % (
    id(otherProto),
    id(a)))

And that would be better.

Changed 11 years ago by SeanMcQ

Attachment: 3695_complete.5.patch added

Changed 11 years ago by SeanMcQ

Attachment: 3695_complete.6.patch added

comment:11 Changed 11 years ago by David Reid

Ok, the only comment I have now is that your assertion doesn't need to be on two lines to be readable in the second test.

comment:12 Changed 11 years ago by David Reid

- ignore that.

Changed 11 years ago by SeanMcQ

Attachment: 3695_complete.7.patch added

comment:13 Changed 11 years ago by SeanMcQ

Owner: SeanMcQ deleted

comment:14 Changed 11 years ago by SeanMcQ

Keywords: review added

comment:15 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone
  1. the if self.innerProtocol: check will "fail" if the protocol has a __nonzero__ that returns False. A silly thing to do, but it's easy to catch this kind of thing and deal with it, so we should. if self.innerProtocol is not None: is generally the form of the right solution.
  2. The repr is implemented in terms of id (it probably was in trunk already, I didn't check). This will produce ugly results sometimes, when the unsigned interpretation of the id is greater than 2 31 - it comes out as a negative Python integer. twisted.python.util.unsignedID works around this problem. (Likewise the test needs to be updated as well).
  3. TestProto's ivars aren't quite formatted right. @ivar foo: bar is correct - you forgot the :.
  4. Even when interpolating a single value, it's best to consistently use a tuple. "%s" % bar is better as "%s" % (bar,). The ',' is necessary to actually make it a tuple. "%s" % (bar) is the same as the first form, but with unnecessary grouping. In the case of id (or unsignedID as I am going to change it to be), the interpolated value is always an integer so this doesn't matter, but using it consistently is a good way to make sure that when it does matter (the case where the interpolated value might be a tuple but you don't know), it gets done right. :)

Otherwise, looks great. These are minor issues. I'm going to fix them and put the result into a branch for someone else to take a quick look at. Thanks! :)

comment:16 Changed 11 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/amp-repr-3695

(In [26626]) Branching to 'amp-repr-3695'

comment:17 Changed 11 years ago by Jean-Paul Calderone

(In [26627]) Apply Rotund's repr fix patch, plus some modifications fixing review feedback

refs #3695

comment:18 Changed 11 years ago by Jean-Paul Calderone

Keywords: easy review added
Owner: Jean-Paul Calderone deleted

Okay, addressed my own review feedback. ;) Someone else please review.

comment:19 Changed 11 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

Looks fine to me - merge.

comment:20 Changed 11 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [26697]) Merge amp-repr-3695

Author: SeanMcQ, exarkun Reviewer: exarkun, glyph Fixes: #3695

Change the repr of twisted.protocols.amp.AMP instances to exclude the inner protocol if one does not exist and to be more explicit about what it is if one does exist.

comment:21 Changed 9 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.