Opened 9 months ago

Last modified 6 months ago

#6862 enhancement new

dns._EDNSMessage should use meaningful constructor argument names and attributes

Reported by: rwall Owned by: rwall
Priority: normal Milestone: EDNS0
Component: names Keywords:
Cc: Branch: branches/better-edns-message-arguments-6862-2
(diff, github, buildbot, log)
Author: rwall Launchpad Bug:

Description

dns._EDNSMessage has been written for compatibility with dns.Message. Which means that it inherits all the horrible abbreviated constructor argument and attribute names.

Maybe that was a mistake. Instead lets try and give dns._EDNSMessage the ideal interface and then provide adaptors for dns.Message if necessary.

Attachments (1)

deprecate-message-6862.patch (64.4 KB) - added by rwall 7 months ago.
Introduce a new "StandardMessage" class with meaningful constructor args and attribute names. Deprecate dns.Message.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 9 months ago by rwall

  • Author set to rwall
  • Branch set to branches/better-edns-message-arguments-6862

(In [40926]) Branching to 'better-edns-message-arguments-6862'

comment:2 Changed 9 months ago by rwall

  • Keywords review added

Ready for review in log:branches/better-edns-message-arguments-6862

  1. I updated the _EDNSMessage constructor arguments
  2. ...and added an alternative constructor to dns.Message which I've provisionally called sensibleConstructor.
  3. ...and added properties to dns.Message which allow the full attribute names to be queried.
  4. I experimented with using twisted.python.constants - it soon got complicated when I tried to share the tests with dns.Message.
    1. And FlagConstants didn't seem appropriate because the Message flags don't need any values.

I'd like some feedback on this approach. Ultimately, I'd like to deprecate the
old cryptic attributes and constructor arguments on dns.Message, so perhaps it
would be better to make properties for the old attribute names which issue
deprecation warnings. But what's the best way to deprecate the dns.Message
constructor argument names?

Build results:

comment:3 Changed 7 months ago by rwall

  • Branch changed from branches/better-edns-message-arguments-6862 to branches/better-edns-message-arguments-6862-2

(In [41140]) Branching to 'better-edns-message-arguments-6862-2'

comment:4 Changed 7 months ago by rwall

I've redone this branch in log:better-edns-message-arguments-6862-2

  • This time without making any changes to dns.Message
  • So I've had to do some further adaptation of the tests; converting constructor argument names before passing them to dns.Message
  • Ultimately, I want dns.Message to have the same meaningful attribute names and constructor arguments, so once this is merged, I'll create a branch to add a deprecate the use the old Message names.
  • or deprecate Message and provide a new alternative (StandardMessage) with good names

Build Results:

Changed 7 months ago by rwall

Introduce a new "StandardMessage" class with meaningful constructor args and attribute names. Deprecate dns.Message.

comment:5 Changed 7 months ago by rwall

In attachment:deprecate-message-6862.patch (against better-edns-message-arguments-6862-2)

  • Hacked together a new "StandardMessage" class which has meaningful constructor arguments and attribute names.
  • It re-uses dns.Message for it's encoding and decoding (until such a time as dns.Message can be removed)
  • Made dns.Message private and introduced a deprecated subclass
  • Refactored the tests so that constructor tests can be shared between the three message classes.
  • Made _EDNSMessage.fromStr a class method -- which is much neater and leads to shorter tests.
  • Introduced a LegacyMessage adaptor so that the inplace version of fromStr in dns.Message can be tested using the same tests as _EDNSMessage and StandardMessage.

There's one failing test -- caused by my commenting out "self.maxSize = 0" in Message.decode.
I commented it out, because it was causing Message comparisons to fail between newly instantiated Messages and decoded messages.

Need to investigate, I suspect a bug.

So anyway, this patch gives an idea about the direction I'm heading. I'm interested to know if it's the right direction.

If so, I can split the patch into separate tickets for

  • Introduction of "StandardMessage"
  • Replace use of Message with StandardMessage within twisted code (possibly one ticket per module)
  • Deprecate dns.Message
  • Remove dns.Message

comment:6 Changed 6 months ago by glyph

  • Keywords review removed
  • Owner set to rwall

So, "I definitely like this direction". Much better docs, I like the less mutation-oriented approach, etc. Are you looking for more guidance than that? My one caveat here would be that StandardMessage is a bit wordy, and it's then not clear what the distinction with Message is until the deprecation is fully done.

I might also make it a bit easier to convert back and forth so that legacy code that is processing Message instances can start moving.

Note: See TracTickets for help on using tickets.