paulc / dnslib

A Python library to encode/decode DNS wire-format packets
https://github.com/paulc/dnslib
BSD 2-Clause "Simplified" License
295 stars 84 forks source link

Add typing / drop Python < 3.7 support #61

Open nhairs opened 5 months ago

nhairs commented 5 months ago

This PR adds Python 3.7+ compatible type annotations (dropping support for <3.7 at the same time).

fixes: #56 #60

At a high level the following changes have been made:

Most major changes in the code are accompanied by Changed in / New in 1.0 in the docstrings.

Test Plan

  1. Ensure that run_tests.sh passes without errors
  2. Ensure that mypy passes without errors.

Notes

Unfortunately there's not much I can do to get GitHub to detect the renamed and edited files (particularly dns.py), it's probably to do with the number of lines changed.... (old screenshot but you get the idea)

image

nhairs commented 5 months ago

This PR is mostly done apart from general cleanup things. I might continue to add type annotations to the non-core libraries.

There are substantial changes (including some that might be controversial) so now would be a good time to get some eyes on it.

cc: @paulc

nhairs commented 5 months ago

@paulc - this is ready for review.

Aside:

I've somewhat been thinking about been thinking about the state of this library compared to some of the alternatives - specifically dnspython which appears to be much more actively maintained, more feature complete (with new DNS features / updates).

It kinda raises the question of it would be better to "deprecate" this package and point other users to the more active package (if yes, then we'd probably just close without committing this PR). This would free up your time mean overall there is less duplicated work between the two communities.

Now that said I've not exhaustively gone through dnspython to know if it covers everything that dnslib does, but thought I'd raise it in case we do want to explore doing such a thing.

Finally this isn't a reflection on the quality of this package, having worked through pretty much the whole code base there's some really elegant code in there.

paulc commented 3 months ago

Apologies for the delay responding to this. I appreciate that a huge amount of work has gone into this PR though as I mentioned I don't really have any time to look at this any more and to be honest I'm not sure that I'm really adding any value in this process. Given your last comment and the scale of the changes I'm slightly reluctant to merge this and would agree that the best answer would probably be to leave dnslib as-is in maintenance/frozen mode (for any downstream software that relies on it) and either for you to fork and create a dnslib2 package based on this PR (I'd be happy to to add a pointer in the README recommending people switch over to this) or recommend that people use a different package.