Opened 5 years ago

Closed 5 years ago

#6057 enhancement closed fixed (fixed)

Port twisted.names.dns to Python 3

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: normal Milestone: Python 3.3 Minimal
Component: names Keywords:
Cc: Branch: branches/dns-py3-6057
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

This is needed for twisted.names.client.

Change History (13)

comment:1 Changed 5 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/dns-py3-6057

(In [35965]) Branching to 'dns-py3-6057'

comment:2 Changed 5 years ago by Jean-Paul Calderone

Keywords: review added
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

comment:3 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone

Thanks for dealing with this.

  1. This is the 2nd time you've implemented a bytes version of chr(), isn't it? I suggest opening a ticket for putting this in compat, though actually doing the ticket isn't urgent.
  2. Returning unicode from Charstr.__str__ etc. on Python 2 seems like a bug.
  3. I love the thing where suffix is unicode but prefix is bytes. Oh well.
  4. News file.
  5. Might be worth doing a coverage test, and maybe opening separate tickets for rarely used classes that need better test coverage before porting. I caught at least the following:
    1. Line 388 seems like '' should be bytes. Missing test to cover this?
    2. Line 1071 (Record_A6.decode) looks like you're concatenating unicode and bytes. Again, missing test? Record_A6 seems undertested in general.

comment:4 Changed 5 years ago by Jean-Paul Calderone

Keywords: review added
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

This is the 2nd time you've implemented a bytes version of chr(), isn't it? I suggest opening a ticket for putting this in compat, though actually doing the ticket isn't urgent.

Filed #6070.

Returning unicode from Charstr.str etc. on Python 2 seems like a bug.

But it works. So...?

I love the thing where suffix is unicode but prefix is bytes. Oh well.

Hee hee. But it basically makes sense, since the suffix and prefix are of difference types in the abstract, too. Also no one uses A6 records. :/

News file.

Misc added in r35996.

Line 388 seems like should be bytes. Missing test to cover this?

Hummm. That's actually covered by the tests. It works because the only use of the '' value after it is assigned to the name local is a test for truth to decide whether to continue the loop. I agree it's confusing that it's a text string, though. Perhaps it should even be None instead of '' or b''. I tweaked the code in r35997.

Line 1071 (Record_A6.decode) looks like you're concatenating unicode and bytes. Again, missing test? Record_A6 seems undertested in general.

Crap. Almost all the record classes are missing test coverage for encoding and decoding. I added some in r35998, and also fixed the Record_A6 bug.

Updated build results

comment:5 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone

Thanks for the improved coverage.

  1. Removing unicode still seems wrong to me, since it violates the API and might break sometime. How about nativeString()? Does the right thing on both 2 and 3.
  2. Add a minimal test or two for str2time, which looks like it's biggest piece of missing coverage.

Once you've done that, please merge.

comment:6 Changed 5 years ago by Jean-Paul Calderone

Removing unicode still seems wrong to me, since it violates the API and might break sometime. How about nativeString()? Does the right thing on both 2 and 3.

Removing it from what?

comment:7 Changed 5 years ago by Itamar Turner-Trauring

Sorry. Lack of sleep is getting to me.

Returning unicode from __str__ still seems wrong to me, since it violates the API and might break something or other. How about nativeString()? Does the right thing on both 2 and 3, and seems to be exactly what you want.

comment:8 Changed 5 years ago by Jean-Paul Calderone

(In [36010]) Use nativeString to return bytes on Python 2 and text on Python 3 (though Python 2 is happy with text here too, through implicit encoding I guess)

refs #6057

comment:9 Changed 5 years ago by Jean-Paul Calderone

(In [36011]) Add some tests for str2time

refs #6057

comment:10 Changed 5 years ago by Jean-Paul Calderone

Okay, did those things. Thanks!

comment:11 Changed 5 years ago by Jean-Paul Calderone

(In [36012]) Okay, I added this bogus documentation in this branch, guess I'll fix it here too.

refs #6057

comment:12 Changed 5 years ago by Jean-Paul Calderone

(In [36013]) Slightly more fixing

refs #6057

comment:13 Changed 5 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [36014]) Merge dns-py3-6057

Author: exarkun Reviewer: itamarst Fixes: #6057

Port twisted.names.dns to Python 3. As part of this, add some missing test coverage and fix minor comparison issues.

Note: See TracTickets for help on using tickets.