Opened 7 years ago
Last modified 7 years ago
#6862 enhancement new
dns._EDNSMessage should use meaningful constructor argument names and attributes
Reported by: | Richard Wall | Owned by: | Richard Wall |
---|---|---|---|
Priority: | normal | Milestone: | EDNS0 |
Component: | names | Keywords: | |
Cc: | Branch: |
branches/better-edns-message-arguments-6862-2
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | rwall |
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)
Change History (7)
comment:1 Changed 7 years ago by
Author: | → rwall |
---|---|
Branch: | → branches/better-edns-message-arguments-6862 |
comment:2 Changed 7 years ago by
Keywords: | review added |
---|
Ready for review in log:branches/better-edns-message-arguments-6862
- I updated the _EDNSMessage constructor arguments
- ...and added an alternative constructor to dns.Message which I've provisionally called sensibleConstructor.
- ...and added properties to dns.Message which allow the full attribute names to be queried.
- I experimented with using twisted.python.constants - it soon got complicated
when I tried to share the tests with dns.Message.
- 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 years ago by
Branch: | branches/better-edns-message-arguments-6862 → branches/better-edns-message-arguments-6862-2 |
---|
(In [41140]) Branching to 'better-edns-message-arguments-6862-2'
comment:4 Changed 7 years ago by
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 years ago by
Attachment: | deprecate-message-6862.patch added |
---|
Introduce a new "StandardMessage" class with meaningful constructor args and attribute names. Deprecate dns.Message.
comment:5 Changed 7 years ago by
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 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Richard Wall |
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.
(In [40926]) Branching to 'better-edns-message-arguments-6862'