Opened 2 years ago

Closed 2 years ago

#6057 enhancement closed fixed (fixed)

Port twisted.names.dns to Python 3

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone: Python 3.3 Minimal
Component: names Keywords:
Cc: Branch: branches/dns-py3-6057
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

This is needed for twisted.names.client.

Change History (13)

comment:1 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/dns-py3-6057

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

comment:2 Changed 2 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to itamar

comment:3 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner changed from itamar to exarkun

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 2 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to itamar

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 2 years ago by itamar

  • Keywords review removed
  • Owner changed from itamar to exarkun

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 2 years ago by exarkun

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 2 years ago by itamar

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 2 years ago by exarkun

(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 2 years ago by exarkun

(In [36011]) Add some tests for str2time

refs #6057

comment:10 Changed 2 years ago by exarkun

Okay, did those things. Thanks!

comment:11 Changed 2 years ago by exarkun

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

refs #6057

comment:12 Changed 2 years ago by exarkun

(In [36013]) Slightly more fixing

refs #6057

comment:13 Changed 2 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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.