#8259 enhancement closed fixed (fixed)

port twisted.names.authority to Python 3

Reported by: Pawel Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: names Keywords:
Cc: Branch: branches/t-names-authority-py3-8259-3
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl, adiroiban

Description

See previous ticket #8195 (ticket)

Attachments (1)

8259.patch (16.0 KB) - added by Pawel 21 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 21 months ago by Pawel

Owner: set to Pawel

Changed 21 months ago by Pawel

Attachment: 8259.patch added

comment:2 Changed 21 months ago by Pawel

Keywords: review added
Milestone: Python-3.x
Owner: Pawel deleted

Here's the patch. It ports twisted.names.authority and twisted.names.secondary + it ports twisted.test.test_names tests and twisted.names.test_examples tests.

Fixes:

  • usual trivial Python 3 porting stuff, e.g. replacing StringIO with NativeStringIO, removing deprecated syntax where it matters
  • ip addresses should be string (not bytes) because this is what socket.inet_aton accepts on Python 3 (it was accepting bytes on Python 2)
  • domain names should be bytes
  • DNS records inheriting from TXT record need to accept bytes as some arguments to init

let me know if you find any issues

comment:3 Changed 21 months ago by Pawel

Owner: set to Adi Roiban

comment:4 Changed 21 months ago by Adi Roiban

Author: adiroiban
Branch: branches/t-names-authority-py3-8259

(In [47122]) Branching to t-names-authority-py3-8259.

comment:5 Changed 21 months ago by Adi Roiban

Owner: Adi Roiban deleted

It looks good

some pyflakes and twistedchecker error

************* Module twisted.names.test.test_names
C0301:352,0: Line too long (91/79)
C0301:363,0: Line too long (100/79)

twisted/names/test/test_names.py:12: 'NativeStringIO' imported but unused

branch created for branch and patch sent to buildbot.

I don't have time now to investigate if we should have bytes or unicode...but please use explicit text marker that is use u'text' or b'text' when changing types

Leaving this for review so that another persons should check it

I would argue that the API should also accept bytes... but I have never used twisted.named and I don't have a good understanding of DNS APIs.

Thanks for the patch!

comment:6 Changed 21 months ago by hawkowl

Owner: set to hawkowl

comment:7 Changed 21 months ago by hawkowl

Keywords: review removed

Hi, thanks for this patch, pawelmhm.

The bytes/Unicode separation issues are a bit complex; I agree with Adi's "also accepting bytes", and I'm also wary of the fact that Twisted handles IP addresses and URLs as "text" (i.e. unicode) in most parts of Twisted on Python 3. Since a lot of this is tiny changes and a lot of finicky stuff, writing the review so you could change it would take more time for me to write and also waste your time, so I went ahead and built off your changes.

After reading through the code, and thinking on it, I think that handling bytes and Unicode strings is important wherever we handle (just) URLs or IP addresses, and so we should accept Unicode and bytes on both platforms, encoding using idna or ascii where required (as both URLs and IP addresses should be ASCII). As such, I've made the tests use a mixture of declared bytes (what it serves) and undeclared str (so, Unicode on Python 3) in the comparisons, to ensure that both are handled correctly and that they serialise down to the same value. To this end, I've done a lot of little docstrings that say that they accept bytes or unicode -- but only in the __init__ - the instance variables are all always bytes, in a binary packed format (so, something you send over the wire, an encoded version of what you give to the init).

The patch has grown quite big as a result of lots of little line changes along the way (e.g. autoformatting by python-docstring-mode), so I will whittle it down so it's reviewable.

comment:8 Changed 21 months ago by hawkowl

Author: adiroibanhawkowl, adiroiban
Branch: branches/t-names-authority-py3-8259branches/t-names-authority-py3-8259-2

(In [47188]) Branching to t-names-authority-py3-8259-2.

comment:9 Changed 21 months ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

I'm happy with this. Since I did a lot of changes (mostly adding docstrings), I'll put it up for review. This patch should catch all of the ambiguous types taken by it, but leaves in a lot of bare strs in the tests so that handling those is handled too.

https://twistedmatrix.com/trac/ticket/8279 is the cause of the two Fedora 23 2.7 builders failing, so you can disregard those.

comment:10 Changed 20 months ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Looks good; lots of new docstrings, ResourceWarning fixes, type fixes. Thanks.

Two minor issues you might want to fix before landing:

  1. You left a few C{bytes} in various places where you changed str to bytes; particularly jarring when it's now C{bytes} or L{unicode}.
  2. Maybe put a comment in as to why you're using NativeStringIO for stdin/stdout in tests? If find the ioType of sys.stdout sort of baffling so some redundancy there never hurts :).
  3. Since #8279 is fixed, maybe merge forward just to get a pre-commit full test run just to be sure.

Otherwise, please land!

comment:11 Changed 20 months ago by hawkowl

Branch: branches/t-names-authority-py3-8259-2branches/t-names-authority-py3-8259-3

(In [47223]) Branching to t-names-authority-py3-8259-3.

comment:12 Changed 20 months ago by hawkowl

Resolution: fixed
Status: newclosed

(In [47227]) Merge t-names-authority-py3-8259-3: Port twisted.names.authority to Python 3

Author: pawelmhm, hawkowl Reviewers: adiroiban, hawkowl, glyph Fixes: #8259

Note: See TracTickets for help on using tickets.