Opened 3 years ago

Closed 3 years ago

#6847 enhancement closed fixed (fixed)

twisted.names.dns.Message should have a meaningful repr

Reported by: rwall Owned by: rwall
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/meaningful-message-repr-6847-2
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

dns.Message is difficult to debug because it used the default repr.

In [2]: from twisted.names.dns import Message

In [3]: m = Message()

In [4]: repr(m)
Out[4]: '<twisted.names.dns.Message instance at 0x2dca878>'

It would be nice to include all the fields values in the repr.

Easiest way is to inherit from FancyStrMixin

Which is what we've done with the proposed EDNSMessage class here:

Change History (14)

comment:1 Changed 3 years ago by rwall

  • Author set to rwall
  • Branch set to branches/meaningful-message-repr-6847

(In [40841]) Branching to 'meaningful-message-repr-6847'

comment:2 Changed 3 years ago by rwall

  • Keywords review added

Ready for review in log:branches/meaningful-message-repr-6847

  • Used FancyStrMixin to add all the field values and the section lists to the message repr

Build results:

comment:3 Changed 3 years ago by rwall

See also #6848 for rich comparison of dns.Messages

comment:4 Changed 3 years ago by hawkowl

  • Owner set to hawkowl

comment:5 Changed 3 years ago by hawkowl

  • Keywords review removed
  • Owner changed from hawkowl to rwall

Looks good - instead of that useless message, I now get...

>>> repr(m)
'<Message id=0 answer=0 opCode=0 auth=0 trunc=0 recDes=0 recAv=0 rCode=0 authenticData=0 checkingDisabled=0 maxSize=512 queries=[] answers=[] authority=[] additional=[]>'

...which is a lot more useful.

The TwistedChecker error in the build is spurious and every good VM passes, so, looks good to me. +1 for merge.

comment:6 Changed 3 years ago by exarkun

That's awfully verbose, isn't it?

That particular message seems to be represented just as completely and much more succinctly as:

<Message id=0 maxSize=512>

comment:7 Changed 3 years ago by rwall

It is a bit verbose, but the reason I'm interested in this, is to make it easier to see the differences when comparing two messages in unit tests.

I've used the same verbose repr in the EDNSMessage branch (#5675). Having all the field values in the repr made it much more obvious what had gone wrong if eg I hadn't bit shifted one of the flags correctly.

Another argument for the verbose repr is that it shows all the field values which are used in the proposed new rich comparison...#6848.

Perhaps, the answer is to implement a custom assertion -- assertMessageIdentical....or even a general assertion for comparing all the public attributes of two objects....assertIdenticalAttributes.

Such an assertion could print out only the differences between the two objects, which would be even clearer.

Regarding this branch, I wonder why you prefer to include the maxSize attribute? If a message is being logged and printed for debugging I think I'd like to at least see the values of all the flags.

Maybe I could use a comma separated list flags which are set and include a section count rather than the full list of records in each populated section. eg

'<Message id=0 rCode=1 opCode=1 flags=answer,auth,trunc maxSize=512 queries=1 answers=1>'

comment:8 Changed 3 years ago by exarkun

Sorry, perhaps I could have made my comment clearer. What I tried to demonstrate with the example is that many of the fields of that particular message instance didn't have interesting values and that uninteresting values could be omitted.

Afterwards I did realize that 512 is probably the default maxSize so it isn't very interesting. ;)

So, here's a more specific suggestion: only render fields that have values that differ from the default (where a default exists). I do like the list of flag values you gave - which is sort of the same idea (but with special rendering logic for things that are bits in a field) - flags that aren't set won't be shown.

I'm not sure queries=1 is a good idea though. Two messages may be identical except contain different queries and this makes them drastically different - that difference may still be worth reflecting here? It seems like there's likely to be more room to do so once all of the other clutter (boring default values) is dropped.

comment:9 Changed 3 years ago by rwall

  • Keywords review added
  • Owner rwall deleted

Ready for another review in log:branches/meaningful-message-repr-6847

  • Thanks for the review comments hawkowl and exarkun
  • I've re-implemented it without FancyStrMixin.
  • It now only shows flags which are True, and fields and sections that are non-default.
  • Id is always shown.
  • Added a few more tests for the various cases.

Build Results:

comment:10 Changed 3 years ago by lewq

  • Owner set to lewq

comment:11 follow-up: Changed 3 years ago by lewq

  • Keywords review removed

Thanks for this rwall! This looks good, only a minor nit:

  1. _flagNames = ('answer', needs a space after it.
  2. Is it convention to explicitly name byte strings as b""? If so please replace all native string types with bytes. If this is not mandatory for new Twisted code, then ignore this comment.

Finally I presume that the two failing builders are normally failing and this branch doesn't make them worse. Please address the above and then merge!

comment:12 Changed 3 years ago by rwall

  • Branch changed from branches/meaningful-message-repr-6847 to branches/meaningful-message-repr-6847-2

(In [41385]) Branching to 'meaningful-message-repr-6847-2'

comment:13 in reply to: ↑ 11 Changed 3 years ago by rwall

  • Owner changed from lewq to rwall
  • Status changed from new to assigned

Thanks Luke,

I've merged forward and addressed your code review comments.

If the buildbots go green I'll merge it.

Build results:

Some more comments below:

Replying to lewq:

Thanks for this rwall! This looks good, only a minor nit:

  1. _flagNames = ('answer', needs a space after it.

r41387.

  1. Is it convention to explicitly name byte strings as b""? If so please replace all native string types with bytes. If this is not mandatory for new Twisted code, then ignore this comment.

As I understand it, the names of variables listed in _flagNames, _fieldNames etc should be native strings.

And the repr should be a native string too.

So I'll leave it as is.

Finally I presume that the two failing builders are normally failing and this branch doesn't make them worse. Please address the above and then merge!

That's normal.

-RichardW.

comment:14 Changed 3 years ago by rwall

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [41393]) Merge meaningful-message-repr-6847-2

Author: rwall Reviewers: hawkowl, exarkun, lewq Fixes: #6847

twisted.names.dns.Message now has a repr method which shows only those instance flags, fields and sections which are set to non-default values.

Note: See TracTickets for help on using tickets.