Opened 2 years ago

Last modified 17 months ago

#5895 task new

Deprecate inet_pton, inet_ntop, and adict in twisted.python.compat

Reported by: itamar Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/inet-compat-5895
(diff, github, buildbot, log)
Author: mateusbraga Launchpad Bug:

Description

inet_pton, inet_ntop, and adict in twisted.python.compat are no longer necessary (2.6 has good implementations of first two, third is just junk). They should be deprecated.

Attachments (5)

ticket5895.patch (10.0 KB) - added by mateusbraga 19 months ago.
Patch to solve this ticket.
ticket5895_2.patch (10.5 KB) - added by mateusbraga 18 months ago.
twisted/topfiles/5895.removal added to the patch
ticket5895_3.patch (10.7 KB) - added by mateusbraga 18 months ago.
Included some of thijs suggestions
ticket5895_4.patch (11.4 KB) - added by mateusbraga 18 months ago.
Included thijs suggestions
ticket5895_5.patch (506 bytes) - added by mateusbraga 17 months ago.
Patch to pass test

Download all attachments as: .zip

Change History (24)

comment:1 Changed 19 months ago by itamar

  • Keywords easy added

See twisted/python/reflect.py and twisted/test/test_reflect.py for an example of deprecation and testing of deprecation.

Changed 19 months ago by mateusbraga

Patch to solve this ticket.

comment:2 Changed 19 months ago by mateusbraga

  • Keywords review added

comment:3 Changed 19 months ago by mateusbraga

How can I create a patch file that contains the new twisted/topfiles/5895.removal file that I created?

"svn diff -x -u > my-twisted-patch.patch" did not work in my case.

twisted/topfiles/5895.removal should have:

twisted.python.compat.adict is now deprecated.
twisted.python.compat.inet_pton is now deprecated in favor of socket.inet_pton from the standard library.
twisted.python.compat.inet_ntop is now deprecated in favor of socket.inet_ntop from the standard library.

comment:4 Changed 18 months ago by dhanush

  • Keywords review removed
  • Owner set to mateusbraga

You need to do the following for your 5895.removal file to show up in your patch :

  1. svn add twisted/topfiles/5895.removal
  1. svn diff

Also consider going through this : http://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch

Changed 18 months ago by mateusbraga

twisted/topfiles/5895.removal added to the patch

comment:5 Changed 18 months ago by mateusbraga

  • Keywords review added
  • Owner mateusbraga deleted

comment:6 Changed 18 months ago by Julian

Leaving the review keyword for a more compete review, but if the socket module functions pass the unit tests we can remove the implementation entirely in favor of just using the stdlib stuff already and just leave the deprecation for accessing the names through this module.

comment:7 Changed 18 months ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to mateusbraga
  • Type changed from enhancement to task

Thanks for your patch.

  1. the deprecated inet_pton and inet_ntop functions don't have a docstring: it would be nice to add that explaining why they are there, what they try to accomplish (something that is already present in the stdlib nowadays?)
  2. in twisted.test.test_compat.DeprecationTestCase:
    1. the class docstring contains a reference to twisted.python.compat that should be wrapped in L{} and end with a period: L{twisted.python.compat}.
    2. you should avoid using the word Test in the docstring of the tests.
    3. C{str} should be L{str}
  3. I would try to rewrite the news file into a single sentence, or at least prevent multiple lines like that

Changed 18 months ago by mateusbraga

Included some of thijs suggestions

comment:8 follow-up: Changed 18 months ago by mateusbraga

  • Keywords review added
  • Owner mateusbraga deleted

I kept C{str} instead of L{str} because according to http://epydoc.sourceforge.net/epytext.html C{} is for python identifiers. (twisted/test/test_reflect.py also uses C{} instead of L{} for str)

Did I get it right?

comment:9 in reply to: ↑ 8 Changed 18 months ago by thijs

  • Keywords review removed
  • Owner set to mateusbraga

Replying to mateusbraga:

I kept C{str} instead of L{str} because according to http://epydoc.sourceforge.net/epytext.html C{} is for python identifiers. (twisted/test/test_reflect.py also uses C{} instead of L{} for str)

I was confused by this as well but it turns out it's an (undocumented) feature of pydoctor, which uses epytext behind the scenes but made an exception for L{str} and other builtins. Here's the IRC chat we had about this recently:

 oh, can't you reference params with L{}?
<rwall> It ends up being a link to some API documentation - either in twisted or in stdlib
<jonathanj> that's unfortunate
<rwall> So L has to refer to something imported
<thijstriemstra> ok so L{str}  is wrong as well
<jonathanj> well, no
<kenaan> jonathanj r38147 M(branches/browser-like-redirect-agent-5434/twisted/web/client.py): Fix invalid epytext. ...
<jonathanj> L{} has some special hax for builtins
<thijstriemstra> oh..
<jonathanj> (as an addition to pydoctor)
<rwall> oh ok. Yeah.
<jonathanj> which is probably why you won't find it documented anywhere :(
<jonathanj> rwall: thanks for the review, btw
<rwall> np
<exarkun> Why is L{builtin} surprising?
<exarkun> It's not like `str` is ever /not/ in scope.  Why wouldn't L{} work with it?
<jonathanj> probably because the epydoc epytext documentation doesn't mention anything about it
<jonathanj> the list of searches taken in the epytext docs don't mention stuff about lexical scope
<jonathanj> maybe this could mean that:  Next, epydoc looks for an object with the given name in the current module.

Did I get it right?

  1. So switch those C{str} to L{str}
  2. Open a ticket to deprecate set and frozenset in twisted.python.compat (they're not useful any longer) and add to keyword easy to that (and feel free to start working on it of course :)
  3. Thank for adding the docstrings to the deprecated methods, but you forgot to document its parameters
  4. in docstrings things like inet_ntop() should be wrapped in C{} to indicate its a code reference
  5. there need to be 3 empty lines between functions (eg. inet_pton and inet_ntop are currently separated by 1)
  6. you haven't addressed the point from the previous review about the test docstrings yet. For example 'Test deprecation of L{compat.inet_ntop}.' shouldn't include the word 'test', so instead I would write 'L{compat.inet_ntop} is deprecated.' or 'L{compat.inet_ntop} emits a C{DeprecationWarning}.'
  7. This second patch doesn't include the .removal file anymore?

Changed 18 months ago by mateusbraga

Included thijs suggestions

comment:10 Changed 18 months ago by mateusbraga

  • Keywords review added
  • Owner mateusbraga deleted

Thank you for the patience! I think I included everything now.

comment:11 Changed 18 months ago by thijs

  • Author set to thijs
  • Branch set to branches/inet-compat-5895

(In [38411]) Branching to 'inet-compat-5895'

comment:12 Changed 18 months ago by thijs

(In [38412]) apply ticket5895_4.patch, refs #5895

comment:13 Changed 18 months ago by thijs

  • Author changed from thijs to mateusbraga
  • Keywords review removed
  • Owner set to mateusbraga

Thanks for your patch, I applied it in a branch. When running admin/run-python3-tests the test_inet_ntop test failed with Python3.3:

======================================================================
ERROR: test_inet_ntop (twisted.test.test_compat.DeprecationTestCase)
test_inet_ntop
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/thijs/workspaces/opensource/software/twisted/svn/Twisted/branches/inet-compat-5895/twisted/internet/defer.py", line 137, in maybeDeferred
    result = f(*args, **kw)
  File "/home/thijs/workspaces/opensource/software/twisted/svn/Twisted/branches/inet-compat-5895/twisted/internet/utils.py", line 203, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/home/thijs/workspaces/opensource/software/twisted/svn/Twisted/branches/inet-compat-5895/twisted/python/compat.py", line 186, in reraise
    raise exception.with_traceback(traceback)
  File "/home/thijs/workspaces/opensource/software/twisted/svn/Twisted/branches/inet-compat-5895/twisted/internet/utils.py", line 199, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/home/thijs/workspaces/opensource/software/twisted/svn/Twisted/branches/inet-compat-5895/twisted/test/test_compat.py", line 641, in test_inet_ntop
    compat.inet_ntop, socket.AF_INET, "\x01\x00\x01\x00")
  File "/home/thijs/workspaces/opensource/software/twisted/svn/Twisted/branches/inet-compat-5895/twisted/trial/_synctest.py", line 1119, in callDeprecated
    result = f(*args, **kwargs)
  File "/home/thijs/workspaces/opensource/software/twisted/svn/Twisted/branches/inet-compat-5895/twisted/python/deprecate.py", line 271, in deprecatedFunction
    return function(*args, **kwargs)
  File "/home/thijs/workspaces/opensource/software/twisted/svn/Twisted/branches/inet-compat-5895/twisted/python/compat.py", line 390, in inet_ntop
    return socket.inet_ntoa(addr)
TypeError: 'str' does not support the buffer interface

----------------------------------------------------------------------
Ran 3196 tests in 7.885s

FAILED (errors=1, skipped=952)

Can you take a look at this and create future patches from the new branch?

Changed 17 months ago by mateusbraga

Patch to pass test

comment:14 Changed 17 months ago by mateusbraga

  • Keywords review added
  • Owner mateusbraga deleted

I just changed so now the test works both on python3.3 and python2.7. I changed from str to bytes. I am not completely sure about how should tests work on both versions, but it is working now.

comment:15 Changed 17 months ago by therve

(In [38583]) Apply patch

Refs #5895

comment:16 Changed 17 months ago by therve

  • Keywords easy review removed
  • Owner set to mateusbraga

I forced a build which quickly showed that python may have inet_pton, but only on some platforms. Can we really deprecate those?

comment:17 Changed 17 months ago by mateusbraga

  • Owner mateusbraga deleted

I don't know if we can really deprecate them anymore, so I will set the owner to nobody.

comment:18 Changed 17 months ago by exarkun

Windows has InetPton which the Python standard library doesn't seem to bother to expose. It might be nice if our compatibility function were implemented in terms of this instead of being a sort of bad stand-alone re-implementation (probably a not very good one).

Alternatively, we should contribute an InetPton-based implementation to the Python standard library so this ticket will be valid later on.

I agree that it appears we cannot get rid of these functions yet.

comment:19 Changed 17 months ago by Julian

There is an open ticket for adding it to the stdlib. It's http://bugs.python.org/issue7171

Note: See TracTickets for help on using tickets.