Opened 5 years ago

Closed 4 years ago

#6909 enhancement closed wontfix (wontfix)

Add iter{keys,values,items} methods to twisted.python.compat

Reported by: real Owned by: Julian Berman
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Branch: branches/itercompat-6909
branch-diff, diff-cov, branch-cov, buildbot
Author: julian

Description

Adding needed functionality to python.compat module, to be used across other modules to make sure the codebase runs on both python2.7 and python3.3

Attachments (1)

python_compat_add_iters.patch (1.2 KB) - added by real 5 years ago.
Addition of iterkeys, itervalues and iteritems to compat.py

Download all attachments as: .zip

Change History (7)

Changed 5 years ago by real

Addition of iterkeys, itervalues and iteritems to compat.py

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

Keywords: python3 patch review removed
Owner: set to real

Thanks. This is missing unit tests and a news fragment.

comment:2 Changed 5 years ago by julian

Author: julian
Branch: branches/itercompat-6909

(In [41341]) Branching to itercompat-6909.

comment:3 Changed 5 years ago by Julian Berman

Keywords: review added
Owner: real deleted
Summary: Extending python.compat moduleAdd iter{keys,values,items} methods to twisted.python.compat

comment:4 Changed 5 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

Reviewing...

comment:5 Changed 5 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Julian Berman
Status: assignednew

Thanks, realcr and Julian,

This all looks pretty good.

Notes:

  1. It would be nice to know which ticket depends on these new wrappers.
  2. Branch merges cleanly
  3. Forced build: https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/itercompat-6909

Points:

  1. Minor whitespace issues
    1. https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1794/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
  2. source:branches/itercompat-6909/twisted/test/test_compat.py
    1. FakeMapping
      1. Why this is this fake necessary. Why not just use dictionaries in the tests?
        1. Add the reasononing as a comment in the docstring or else consider using dict.
        2. FWIW Six seems to use dict https://bitbucket.org/gutworth/six/src/38d0a84dfda54318f877f5ad62372a68c803f202/test_six.py?at=default#cl-354
    2. IterMethodsTest
      1. Shouldn't the tests specifically demonstrate that the wrappers return an iterator rather than a list? You could combine the two assertions into one assertEqual. I've included a sketch in the diff below.
        1. Six seems instead to check that the return value is *not* a list which doesn't seem great. https://bitbucket.org/gutworth/six/src/38d0a84dfda54318f877f5ad62372a68c803f202/test_six.py?at=default#cl-368
      2. source:branches/itercompat-6909/twisted/python/compat.py
        1. The change to _PY3 looks neat, but the six method is maybe even neater: https://bitbucket.org/gutworth/six/src/38d0a84dfda54318f877f5ad62372a68c803f202/six.py?at=default#cl-33
          1. But is that change really necessary in this branch?
        2. I think you should try and add API docs for these new wrappers. Not sure whether that will require them to be defined as proper functions. There are already too many undocumented functions in t.p.compat: https://twistedmatrix.com/documents/current/api/twisted.python.compat.html
          1. Here's how six do it https://bitbucket.org/gutworth/six/src/38d0a84dfda54318f877f5ad62372a68c803f202/six.py?at=default#cl-478
   Index: twisted/test/test_compat.py
   ===================================================================
   --- twisted/test/test_compat.py (revision 41688)
   +++ twisted/test/test_compat.py (working copy)
   @@ -107,7 +107,12 @@
            del keys, values, items


+import collections

+def FakeMapping():
+    return {1:3, 2:4}
+
+
 class IterMethodsTest(unittest.SynchronousTestCase):
     """
     C{iterkeys}, C{itervalues} and C{iteritems} are functions that
@@ -120,7 +125,11 @@
         C{iterkeys} calls C{iterkeys} on Python 2 and C{keys} on Python 3.
         """
         from twisted.python.compat import iterkeys
-        self.assertEqual(list(iterkeys(FakeMapping())), [1, 2])
+        iterator = iterkeys(FakeMapping())
+        self.assertEqual(
+            (True, [1, 2]),
+            (isinstance(iterator, collections.Iterator), list(iterator))
+        )

Anyway, thanks for your work on this. Please answer or address the points above and resubmit for another review with a link to some clean build results.

comment:6 Changed 4 years ago by Julian Berman

Resolution: wontfix
Status: newclosed

Apparently we're finally deciding to move to six in #7177, so this can go away.

Note: See TracTickets for help on using tickets.