Opened 2 years ago

Last modified 18 months ago

#8026 enhancement new

Cleanup of Names

Reported by: Todd Crane Owned by: Todd Crane
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch:
Author: toddjcrane

Description

Cleaned up of names, specifically dns.py

  • Added record and query types up through #52 (TLSA)
  • Reordered Record_* classes according to RR id # (according to IANA)
  • Created AddressRecord for use with Record_A and Record_AAAA

Attachments (2)

names-cleanup-8025.patch (41.7 KB) - added by Todd Crane 2 years ago.
names-cleanup-8026.diff (37.8 KB) - added by Todd Crane 21 months ago.
Fixed patch

Download all attachments as: .zip

Change History (13)

Changed 2 years ago by Todd Crane

Attachment: names-cleanup-8025.patch added

comment:1 Changed 2 years ago by Todd Crane

Keywords: review added

comment:2 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to Todd Crane

Many thanks for your contribution!

This is one big cleanup + adding new query types + refactoring Record_A and Record_AAAA

To me it does more than the ticket's description 'Cleanup of Names`

I suggest to split this patch into 3 tickets.... (or create 2 more tickets)

For each ticket mare sure it has a descriptive title and a description which describes the scope of this ticket.

Please resubmit only the cleanup patch and submit the other 2 tasks as separate ticket.

Non-cleanup tickets needs test coverage to describe the new usage and make sure that future changes don't introduce regressions.

Many thanks!

Changed 21 months ago by Todd Crane

Attachment: names-cleanup-8026.diff added

Fixed patch

comment:3 Changed 21 months ago by Todd Crane

Keywords: review added

Please see new attachment for required fixes

comment:4 Changed 21 months ago by Todd Crane

Owner: Todd Crane deleted

comment:5 Changed 21 months ago by Adi Roiban

Thanks for the patch.

Is the ticket description still valid?

I assume "Created AddressRecord for use with Record_A and Record_AAAA" was moved to #8278

Not sure what happen with 'Added record and query types up through 52 (TLSA)'

The move created one big ugly diff :(

comment:6 Changed 20 months ago by Todd Crane

Can somebody do something about this spam filter? It is blocking my responses

comment:7 Changed 20 months ago by Todd Crane

Yes "Created AddressRecord for use with Record_A and Record_AAAA" was moved to #8278

comment:8 Changed 20 months ago by Todd Crane

Record types will be submitted after #8026 and #8278 make it to trunk

comment:9 Changed 20 months ago by Todd Crane

The ugliness is from ordering the RR Types

comment:10 in reply to:  6 Changed 18 months ago by Glyph

Replying to toddjcrane:

Can somebody do something about this spam filter? It is blocking my responses

Sorry about that. I've been trying to keep up with it; I've flagged all your responses as ham in the past so hopefully it won't give you so much trouble in the future.

comment:11 Changed 18 months ago by Glyph

Keywords: review removed
Owner: set to Todd Crane

Hi toddjcrane,

Thanks for your continued patience and your several contributions to Twisted.

This diff is still pretty big and hard to review - is it supposed to be purely a reordering of Record_* classes?

I'm curious about your motivation for that re-ordering; it isn't preserved at runtime, so it's purely a convenience for maintainers, correct? It strikes me that the effort required to move them around (especially to make sure the patch isn't inadvertently making any other changes at the same time) might not be worth the benefit. But if it is, landing / reviewing a series of small changes would be a lot faster - moving one class at a time into the correct position, for example.

I'll have a look at the other two patches.

Officially, my feedback is:

  1. At 1188 lines, this patch is bordering on "too large to review", and it would be helpful if you could split it up into smaller changes.
  2. Unfortunately, this no longer applies cleanly on trunk (another risk of large patches).

Please address these issues and re-submit.

One thing to consider as you re-submit; although you still need to use Trac to get your patch into the review queue, we now can accept github pull requests as the thing-to-review rather than just diffs. It would definitely be easier to review this, even as a big individual change, if it were a neatly stacked set of individual commits which could each be reviewed one at a time, so preserving git history on the submission would be especially helpful here.

Thanks again!

Note: See TracTickets for help on using tickets.