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

[itamar]

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.

[itamar]

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

[mateusbraga]

Patch to solve this ticket.

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

[dhanush]

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

1. svn add twisted/topfiles/5895.removal

2. svn diff

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

[mateusbraga]

twisted/topfiles/5895.removal added to the patch

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

[thijs]

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

[mateusbraga]

Included some of thijs suggestions

[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)

Did I get it right?

[thijs]

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?

[mateusbraga]

Included thijs suggestions

[mateusbraga]

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

[thijs]

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

[thijs]

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

[thijs]

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?

[mateusbraga]

Patch to pass test

[mateusbraga]

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.