#8195 enhancement closed fixed (fixed)

port twisted.names.server to python 3

Reported by: bluec0re Owned by: Pawel
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/names-server-py3-8195-2
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description (last modified by Glyph)

See previous tickets: #6093 #6056 #6059 #6057 #6058 #6092

Attachments (3)

o (930 bytes) - added by Pawel 21 months ago.
patch.diff (1.8 KB) - added by Pawel 21 months ago.
path.diff (3.1 KB) - added by Pawel 21 months ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 23 months ago by Glyph

Component: corenames
Description: modified (diff)
Summary: twisted.names.server missing in python3 installport twisted.names.server to python 3
Type: defectenhancement

I've added this to wiki:Plan/Python3.

Changed 21 months ago by Pawel

Attachment: o added

comment:2 Changed 21 months ago by Pawel

Keywords: review added
Owner: set to Adi Roiban

this is actually pretty tiny change, here's the patch.

comment:3 Changed 21 months ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Pawel

Thanks for the patch.

Please also check all the tests related to twisted.names.server and also port them (if required) and include them in the python3 test suite. See dist3.py


Also check http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

Your patch requires a news fragment to advertise the new capability as part of the next release notes.


Looking forward for the new patch and have the new tests included into our python3 test suite.

Thanks!

Changed 21 months ago by Pawel

Attachment: patch.diff added

comment:4 Changed 21 months ago by Pawel

Keywords: review added
Owner: changed from Pawel to Adi Roiban

tests seem to pass just fine without changes, I updated dist3.py module list and added news file

comment:5 Changed 21 months ago by Adi Roiban

Author: adiroiban
Branch: branches/names-server-py3-8195

(In [47030]) Branching to names-server-py3-8195.

comment:6 Changed 21 months ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Pawel

Thanks for the patch!


Please note that gotResolverResponse docstring should be updated... it was wrong before but now that we have touched it, it should be a good time to fix the docstring.

I have never used twisted.names so I can not suggest another name for that argument.


To have a closer behaviour between py2.7 and py3 all ported files should do the from __futute__ import for absolute import / division and what other things can be imported :)

I have aleady commted that changes.


server.py advertises that its tests are in

# -*- test-case-name: twisted.names.test.test_names,twisted.names.test.test_server -*-

Since the patch is small, can we have twisted.names.test.test_names also enabled with this patch?


I have applied the patch and send it to buildbot.

Please check my comments and send your feedback.

If possible, please send a patch on top of the branch dedicated to this ticket.

Many thanks for your contribution!

Changed 21 months ago by Pawel

Attachment: path.diff added

comment:7 Changed 21 months ago by Pawel

Owner: changed from Pawel to Adi Roiban

Since the patch is small, can we have twisted.names.test.test_names also enabled with this patch?

unfortunately tests for test_names are failing because of unported authority.py. Should this be ported in this ticket or in new one? I see there is convention of one names module per ticket so maybe would be good to keep this convention?

comment:8 Changed 21 months ago by Pawel

Keywords: review added

is there anything else to do here?

comment:9 Changed 21 months ago by Adi Roiban

Hi,

Sorry for the confusion. You attached a patch without any comment about the patch and I was not sure if more comments will follow.

Is ok to port authority.py in a separate ticket. Please create a ticket and reference it from here.

Will apply the patch and see how it goes.

Thanks!

comment:10 Changed 21 months ago by Adi Roiban

Branch: branches/names-server-py3-8195branches/names-server-py3-8195-2

(In [47094]) Branching to names-server-py3-8195-2.

comment:11 Changed 21 months ago by Pawel

created ticket for authority here #8259

comment:12 Changed 21 months ago by Adi Roiban

Thanks.

Test look good.

There were a couple of minor issues....but hope they are fixed

ex

************* Module twisted.names.server
C0103:264,34:DNSServerFactory.gotResolverResponse: Invalid name "ans_auth_add" for type variable (should match ([a-z_][a-zA-Z0-9]*)$)

I have renamed ans_auth_add to response as I hope it is a better name :)

Waiting for the test results and if buildbot is happy and nobody has any other objection I will merge.

Looking forward for #8259

comment:13 Changed 21 months ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Pawel

Looks good. will merge.

comment:14 Changed 21 months ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [47098]) Merge names-server-py3-8195-2: Port twisted.names.server to Python 3.

Author: pawelmhm Reviewer: adiroiban Fixes: #8195

Note: See TracTickets for help on using tickets.