Drop obsolete private definitions from twisted.python.compat #1363

mthuurne · 2020-07-22T01:10:27Z

Many of the definitions in twisted.python.compat have become obsolete now that Python 2.7 support has been dropped. For private definitions, that means we can drop them as soon as Twisted itself no longer needs them.

Contributor Checklist:

The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/9921
The changes pass minimal style checks (see: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted )
I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
I have updated the automated tests.
I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.

rodrigc · 2020-07-22T16:31:58Z

src/twisted/words/xish/domish.py

@@ -13,8 +13,7 @@

 from zope.interface import implementer, Interface, Attribute

-from twisted.python.compat import (StringType, _coercedUnicode,
-                                   iteritems, itervalues)
+from twisted.python.compat import StringType, iteritems, itervalues


StringType, iteritems, and itervalues can all be easily dropped.
You can do that in this PR, or do a separate one if you want.

rodrigc · 2020-07-22T16:32:39Z

src/twisted/words/protocols/jabber/component.py

@@ -23,7 +23,7 @@
 from twisted.application import service
 from twisted.internet import defer
 from twisted.python import log
-from twisted.python.compat import _coercedUnicode, unicode
+from twisted.python.compat import unicode


unicode can go, and be replaced by str. You can do that in this PR, or a separate one if you want.

rodrigc · 2020-07-22T16:32:52Z

src/twisted/words/protocols/jabber/client.py

@@ -4,7 +4,7 @@
 # See LICENSE for details.


-from twisted.python.compat import _coercedUnicode, unicode
+from twisted.python.compat import unicode


unicode can go

rodrigc · 2020-07-22T16:33:27Z

src/twisted/spread/test/test_banana.py

@@ -9,7 +9,7 @@
 from twisted.trial.unittest import TestCase
 from twisted.spread import banana
 from twisted.python import failure
-from twisted.python.compat import long, iterbytes, _bytesChr as chr
+from twisted.python.compat import long, iterbytes


long can go, and be replaced by int. You can do that in this PR, or a separate one if you want.

rodrigc · 2020-07-22T16:33:46Z

src/twisted/spread/pb.py

@@ -35,8 +35,7 @@

 # Twisted Imports
 from twisted.python import log, failure, reflect
-from twisted.python.compat import (unicode, _bytesChr as chr, range,
-                                   comparable, cmp)
+from twisted.python.compat import unicode, range, comparable, cmp


unicode and range can go

rodrigc · 2020-07-22T16:34:03Z

src/twisted/spread/banana.py

@@ -20,22 +20,27 @@
 from twisted.internet import protocol
 from twisted.persisted import styles
 from twisted.python import log
-from twisted.python.compat import iterbytes, long, _bytesChr as chr
+from twisted.python.compat import iterbytes, long


long can go, replace with int

rodrigc · 2020-07-22T16:35:18Z

src/twisted/mail/smtp.py

@@ -36,8 +36,7 @@
 from twisted.python import log
 from twisted.python import util
 from twisted.python.compat import (long, unicode, networkString,


long, unicode, and iteritems can be easily dropped here

rodrigc · 2020-07-22T16:36:04Z

src/twisted/mail/imap4.py

-    _b64encodebytes as encodebytes,
-    intToBytes, iterbytes, long, nativeString, networkString, unicode,
-    _matchingString, _get_async_param,
+    unichr as chr, intToBytes, iterbytes, long, nativeString, networkString,


long and unicode can go

rodrigc · 2020-07-22T16:37:01Z

src/twisted/conch/ssh/keys.py

-    iterbytes, long, izip, nativeString, unicode,
-    _b64decodebytes as decodebytes, _b64encodebytes as encodebytes,
-    _bytesChr as chr)
+from twisted.python.compat import iterbytes, long, izip, nativeString, unicode


long and unicode can go

rodrigc · 2020-07-22T16:37:14Z

src/twisted/conch/ssh/connection.py

@@ -17,8 +17,7 @@
 from twisted.conch import error
 from twisted.internet import defer
 from twisted.python import log
-from twisted.python.compat import (
-    nativeString, networkString, long, _bytesChr as chr)
+from twisted.python.compat import nativeString, networkString, long


long can go

rodrigc · 2020-07-22T16:38:53Z

src/twisted/conch/client/default.py

@@ -13,8 +13,7 @@


 from twisted.python import log
-from twisted.python.compat import (
-    nativeString, raw_input, _b64decodebytes as decodebytes)
+from twisted.python.compat import nativeString, raw_input


raw_input can be replaced with input

rodrigc

This looks OK. Since you are removing some obsolete compat definitions, I suggested a few
more that you can easily remove as part of this PR.

mthuurne · 2020-07-22T17:40:02Z

I want to handle the public definitions in a separate PR, because if I understand the compatibility policy correctly, those should all be deprecated instead of removed.

rodrigc · 2020-07-22T18:58:22Z

The ones I mentioned don't need to go through the whole deprecation cycle, since they were part of Python 2 to 3 porting, and Python 2 deprecation was announced over a year ago, with the last Twisted release supporting Python 2 being 20.3.0

mthuurne · 2020-07-22T19:10:31Z

What I'm worried about is some project using Twisted having from twisted.python.compat import long in their code. Even though they don't need the functionality anymore, just removing the name from the compat module would break their code.

rodrigc · 2020-07-22T19:34:57Z

Oh I see what you are saying.
No, what I meant to get across in my feedback is don't remove long, unicode, range from twisted.python.compat

Just remove the usages of those things in the twisted files that I indicated.

mthuurne · 2020-07-22T19:52:00Z

I'd prefer to do that in a separate PR, while also deprecating those names, so we can remove them from compat in the future.

rodrigc · 2020-07-23T00:01:16Z

compat is going to be around for a while yet, because unfortunately it was written as a public module, with publicly exported methods and variables. So it is not easy to know who is using it. In hindsight, it would have been better to have the compat module as a completely private module only to be used internally in Twisted. But it's too late for that now.

I think it would be nice if you could remove those uses of compat as I have suggested in this PR.
I have made similar compat removal passes in other PR's that I have filed.

mthuurne · 2020-07-23T00:20:26Z

While much of compat is obsolete now, there are some cases like the comparable decorator that are not trivial replacements. Should that be deprecated too or is it useful to keep that around to help projects port their code to Python 3?

I have already filed a ticket for the public cleanup of compat and am working on the PR, which will include the changes you requested and more. But it is going to include quite a number of changes, so I would really like to get this merged first instead of having a single giant PR.

rodrigc · 2020-07-23T00:34:15Z

Yes the compat needs to be looked at case by case. comparable is one good example you came up with.

mthuurne · 2020-07-23T04:03:56Z

I created a draft version of the public definitions counterpart to this PR, see #1364. I hope the size of that diff will make it clear why I want to merge this PR with its current scope (private definitions only) instead of including cleanups of public definitions such as long and unicode as well.

We can now use type annotations to verify argument types.

rodrigc · 2020-07-24T22:11:58Z

The feedback that I was suggesting you incorporate into this PR was this:

diff --git a/src/twisted/conch/client/default.py b/src/twisted/conch/client/default.py
index 60416d530..f6ac8a84d 100644
--- a/src/twisted/conch/client/default.py
+++ b/src/twisted/conch/client/default.py
@@ -13,7 +13,7 @@ interact with a known_hosts database, use L{twisted.conch.client.knownhosts}.
 
 
 from twisted.python import log
-from twisted.python.compat import nativeString, raw_input
+from twisted.python.compat import nativeString
 from twisted.python.filepath import FilePath
 
 from twisted.conch.error import ConchError
@@ -307,7 +307,7 @@ class SSHUserAuthClient(userauth.SSHUserAuthClient):
             for prompt, echo in prompts:
                 prompt = prompt.decode("utf-8")
                 if echo:
-                    responses.append(raw_input(prompt))
+                    responses.append(input(prompt))
                 else:
                     responses.append(getpass.getpass(prompt))
         return defer.succeed(responses)
diff --git a/src/twisted/conch/ssh/keys.py b/src/twisted/conch/ssh/keys.py
index 413542600..8f02d46f6 100644
--- a/src/twisted/conch/ssh/keys.py
+++ b/src/twisted/conch/ssh/keys.py
@@ -32,7 +32,7 @@ from pyasn1.type import univ
 from twisted.conch.ssh import common, sexpy
 from twisted.conch.ssh.common import int_from_bytes, int_to_bytes
 from twisted.python import randbytes
-from twisted.python.compat import iterbytes, long, izip, nativeString, unicode
+from twisted.python.compat import iterbytes, izip, nativeString
 from twisted.python.constants import NamedConstant, Names
 from twisted.python.deprecate import _mutuallyExclusiveArguments
 
@@ -124,7 +124,7 @@ def _normalizePassphrase(passphrase):
     Normalization Process for Stabilized Strings using NFKC normalization.
     The passphrase is then encoded using UTF-8.
 
-    @type passphrase: L{bytes} or L{unicode} or L{None}
+    @type passphrase: L{bytes} or L{str} or L{None}
     @param passphrase: The passphrase to normalize.
 
     @return: The normalized passphrase, if any.
@@ -132,7 +132,7 @@ def _normalizePassphrase(passphrase):
     @raises PassphraseNormalizationError: if the passphrase is Unicode and
     cannot be normalized using the available Unicode character database.
     """
-    if isinstance(passphrase, unicode):
+    if isinstance(passphrase, str):
         # The Normalization Process for Stabilized Strings requires aborting
         # with an error if the string contains any unassigned code point.
         if any(unicodedata.category(c) == 'Cn' for c in passphrase):
@@ -199,7 +199,7 @@ class Key(object):
         @rtype: L{Key}
         @return: The loaded key.
         """
-        if isinstance(data, unicode):
+        if isinstance(data, str):
             data = data.encode("utf-8")
         passphrase = _normalizePassphrase(passphrase)
         if type is None:
@@ -562,7 +562,7 @@ class Key(object):
                 raise BadKeyError('RSA key failed to decode properly')
 
             n, e, d, p, q, dmp1, dmq1, iqmp = [
-                long(value) for value in decodedKey[1:9]
+                int(value) for value in decodedKey[1:9]
                 ]
             return cls(
                 rsa.RSAPrivateNumbers(
@@ -576,7 +576,7 @@ class Key(object):
                 ).private_key(default_backend())
             )
         elif kind == b'DSA':
-            p, q, g, y, x = [long(value) for value in decodedKey[1: 6]]
+            p, q, g, y, x = [int(value) for value in decodedKey[1: 6]]
             if len(decodedKey) < 6:
                 raise BadKeyError('DSA key failed to decode properly')
             return cls(
@@ -1334,7 +1334,7 @@ class Key(object):
             a comment.  For private OpenSSH keys, this is a passphrase to
             encrypt with.  (Deprecated since Twisted 20.3.0; use C{comment}
             or C{passphrase} as appropriate instead.)
-        @type extra: L{bytes} or L{unicode} or L{None}
+        @type extra: L{bytes} or L{str} or L{None}
 
         @param subtype: A subtype of the requested C{type} to emit.  Only
             supported for private OpenSSH keys, for which the currently
@@ -1347,14 +1347,14 @@ class Key(object):
 
             Present since Twisted 20.3.0.
 
-        @type comment: L{bytes} or L{unicode} or L{None}
+        @type comment: L{bytes} or L{str} or L{None}
 
         @param passphrase: A passphrase to encrypt the key with.  Only
             supported for private OpenSSH keys.
 
             Present since Twisted 20.3.0.
 
-        @type passphrase: L{bytes} or L{unicode} or L{None}
+        @type passphrase: L{bytes} or L{str} or L{None}
 
         @rtype: L{bytes}
         """
@@ -1369,7 +1369,7 @@ class Key(object):
                 comment = extra
             else:
                 passphrase = extra
-        if isinstance(comment, unicode):
+        if isinstance(comment, str):
             comment = comment.encode("utf-8")
         passphrase = _normalizePassphrase(passphrase)
         method = getattr(self, '_toString_%s' % (type.upper(),), None)
diff --git a/src/twisted/mail/imap4.py b/src/twisted/mail/imap4.py
index 2f0ba9095..2a19cc404 100644
--- a/src/twisted/mail/imap4.py
+++ b/src/twisted/mail/imap4.py
@@ -41,7 +41,7 @@ from twisted.internet import error
 from twisted.internet.defer import maybeDeferred
 from twisted.python import log, text
 from twisted.python.compat import (
-    unichr as chr, intToBytes, iterbytes, long, nativeString, networkString,
+    unichr as chr, intToBytes, iterbytes, nativeString, networkString,
     unicode, _matchingString, _get_async_param,
 )
 from twisted.internet import interfaces
@@ -5030,7 +5030,7 @@ def collapseNestedLists(items):
             i = i.encode("ascii")
         if i is None:
             pieces.extend([b' ', b'NIL'])
-        elif isinstance(i, (int, long)):
+        elif isinstance(i, int):
             pieces.extend([b' ', networkString(str(i))])
         elif isinstance(i, DontQuoteMe):
             pieces.extend([b' ', i.value])
diff --git a/src/twisted/mail/smtp.py b/src/twisted/mail/smtp.py
index 0e4ba2f24..dcb6011df 100644
--- a/src/twisted/mail/smtp.py
+++ b/src/twisted/mail/smtp.py
@@ -35,8 +35,7 @@ from twisted.internet.interfaces import ITLSTransport, ISSLTransport
 from twisted.internet._idna import _idnaText
 from twisted.python import log
 from twisted.python import util
-from twisted.python.compat import (long, unicode, networkString,
-                                   nativeString, iteritems, iterbytes)
+from twisted.python.compat import networkString, nativeString, iterbytes
 from twisted.python.runtime import platform
 
 from twisted.mail.interfaces import (IClientAuthentication,
@@ -914,7 +913,7 @@ class SMTPClient(basic.LineReceiver, policies.TimeoutMixin):
     timeout = None
 
     def __init__(self, identity, logsize=10):
-        if isinstance(identity, unicode):
+        if isinstance(identity, str):
             identity = identity.encode('ascii')
 
         self.identity = identity or b''
@@ -1634,7 +1633,7 @@ class ESMTP(SMTP):
 
     def listExtensions(self):
         r = []
-        for (c, v) in iteritems(self.extensions()):
+        for (c, v) in self.extensions().items():
             if v is not None:
                 if v:
                     # Intentionally omit extensions with empty argument lists
@@ -1879,9 +1878,9 @@ class SMTPSenderFactory(protocol.ClientFactory):
         @param timeout: Period, in seconds, for which to wait for
         server responses, or None to wait forever.
         """
-        assert isinstance(retries, (int, long))
+        assert isinstance(retries, int)
 
-        if isinstance(toEmail, unicode):
+        if isinstance(toEmail, str):
             toEmail = [toEmail.encode('ascii')]
         elif isinstance(toEmail, bytes):
             toEmail = [toEmail]
@@ -2126,11 +2125,11 @@ def sendmail(smtphost, from_addr, to_addrs, msg, senderDomainName=None, port=25,
     @type port: L{int}
 
     @param username: The username to use, if wanting to authenticate.
-    @type username: L{bytes} or L{unicode}
+    @type username: L{bytes} or L{str}
 
     @param password: The secret to use, if wanting to authenticate. If you do
         not specify this, SMTP authentication will not occur.
-    @type password: L{bytes} or L{unicode}
+    @type password: L{bytes} or L{str}
 
     @param requireTransportSecurity: Whether or not STARTTLS is required.
     @type requireTransportSecurity: L{bool}
@@ -2172,13 +2171,13 @@ def sendmail(smtphost, from_addr, to_addrs, msg, senderDomainName=None, port=25,
 
     d = defer.Deferred(cancel)
 
-    if isinstance(username, unicode):
+    if isinstance(username, str):
         username = username.encode("utf-8")
-    if isinstance(password, unicode):
+    if isinstance(password, str):
         password = password.encode("utf-8")
 
     tlsHostname = smtphost
-    if not isinstance(tlsHostname, unicode):
+    if not isinstance(tlsHostname, str):
         tlsHostname = _idnaText(tlsHostname)
 
     factory = ESMTPSenderFactory(
diff --git a/src/twisted/spread/banana.py b/src/twisted/spread/banana.py
index 44f3aee67..aa20684ff 100644
--- a/src/twisted/spread/banana.py
+++ b/src/twisted/spread/banana.py
@@ -20,7 +20,7 @@ from io import BytesIO
 from twisted.internet import protocol
 from twisted.persisted import styles
 from twisted.python import log
-from twisted.python.compat import iterbytes, long
+from twisted.python.compat import iterbytes
 from twisted.python.reflect import fullyQualifiedName
 
 
@@ -43,14 +43,13 @@ def int2b128(integer, stream):
 
 def b1282int(st):
     """
-    Convert an integer represented as a base 128 string into an L{int} or
-    L{long}.
+    Convert an integer represented as a base 128 string into an L{int}.
 
     @param st: The integer encoded in a byte string.
     @type st: L{bytes}
 
     @return: The integer value extracted from the byte string.
-    @rtype: L{int} or L{long}
+    @rtype: L{int}
     """
     e = 1
     i = 0
@@ -341,7 +340,7 @@ class Banana(protocol.Protocol, styles.Ephemeral):
             write(LIST)
             for elem in obj:
                 self._encode(elem, write)
-        elif isinstance(obj, (int, long)):
+        elif isinstance(obj, int):
             if obj < self._smallestLongInt or obj > self._largestLongInt:
                 raise BananaError(
                     "int/long is too large to send (%d)" % (obj,))
diff --git a/src/twisted/spread/pb.py b/src/twisted/spread/pb.py
index 0f47b46fd..7e4f05301 100644
--- a/src/twisted/spread/pb.py
+++ b/src/twisted/spread/pb.py
@@ -35,7 +35,7 @@ from zope.interface import implementer, Interface
 
 # Twisted Imports
 from twisted.python import log, failure, reflect
-from twisted.python.compat import unicode, range, comparable, cmp
+from twisted.python.compat import comparable, cmp
 from twisted.internet import defer, protocol
 from twisted.cred.portal import Portal
 from twisted.cred.credentials import IAnonymous, ICredentials
@@ -777,7 +777,7 @@ class Broker(banana.Banana):
             L{None} if there is no object which corresponds to the given
             identifier.
         """
-        if isinstance(luid, unicode):
+        if isinstance(luid, str):
             luid = luid.encode('utf8')
 
         lob = self.localObjects.get(luid)
@@ -827,7 +827,7 @@ class Broker(banana.Banana):
         @param name: An ID.
         @param object: The object.
         """
-        if isinstance(name, unicode):
+        if isinstance(name, str):
             name = name.encode('utf8')
 
         assert object is not None
@@ -844,7 +844,7 @@ class Broker(banana.Banana):
         @param name: The name to look up.
         @return: An object which maps to the name.
         """
-        if isinstance(name, unicode):
+        if isinstance(name, str):
             name = name.encode('utf8')
 
         return RemoteReference(None, self, name, 0)
@@ -1179,7 +1179,7 @@ class Broker(banana.Banana):
 
         @param objectID: The object ID.
         """
-        if isinstance(objectID, unicode):
+        if isinstance(objectID, str):
             objectID = objectID.encode('utf8')
         refs = self.localObjects[objectID].decref()
         if refs == 0:
diff --git a/src/twisted/spread/test/test_banana.py b/src/twisted/spread/test/test_banana.py
index c27431418..cfd079692 100644
--- a/src/twisted/spread/test/test_banana.py
+++ b/src/twisted/spread/test/test_banana.py
@@ -9,7 +9,7 @@ from io import BytesIO
 from twisted.trial.unittest import TestCase
 from twisted.spread import banana
 from twisted.python import failure
-from twisted.python.compat import long, iterbytes
+from twisted.python.compat import iterbytes
 from twisted.internet import protocol, main
 from twisted.test.proto_helpers import StringTransport
 
@@ -164,7 +164,7 @@ class BananaTests(BananaTestBase):
         banana without changing value and should come out represented
         as an C{int} (regardless of the type which was encoded).
         """
-        for value in (10151, long(10151)):
+        for value in (10151, int(10151)):
             self.enc.sendEncoded(value)
             self.enc.dataReceived(self.io.getvalue())
             self.assertEqual(self.result, 10151)
diff --git a/src/twisted/words/protocols/jabber/client.py b/src/twisted/words/protocols/jabber/client.py
index 016bf3af3..85bc96f39 100644
--- a/src/twisted/words/protocols/jabber/client.py
+++ b/src/twisted/words/protocols/jabber/client.py
@@ -4,7 +4,6 @@
 # See LICENSE for details.
 
 
-from twisted.python.compat import unicode
 from twisted.words.protocols.jabber import error, sasl, xmlstream
 from twisted.words.protocols.jabber.jid import JID
 from twisted.words.xish import domish, utility, xpath
@@ -126,9 +125,9 @@ class IQAuthInitializer(object):
         # Prefer digest over plaintext
         if DigestAuthQry.matches(iq):
             digest = xmlstream.hashPassword(self.xmlstream.sid, password)
-            reply.query.addElement("digest", content=unicode(digest))
+            reply.query.addElement("digest", content=str(digest))
         else:
-            reply.query.addElement("password", content = password)
+            reply.query.addElement("password", content=password)
 
         d = reply.send()
         d.addCallbacks(self._cbAuth, self._ebAuth)
@@ -275,7 +274,7 @@ class BindInitializer(xmlstream.BaseFeatureInitiatingInitializer):
 
     def onBind(self, iq):
         if iq.bind:
-            self.xmlstream.authenticator.jid = JID(unicode(iq.bind.jid))
+            self.xmlstream.authenticator.jid = JID(str(iq.bind.jid))
 
 
 
@@ -311,7 +310,7 @@ def XMPPClientFactory(jid, password, configurationForTLS=None):
     @type jid: L{jid.JID}
 
     @param password: password to authenticate with.
-    @type password: L{unicode}
+    @type password: L{str}
 
     @param configurationForTLS: An object which creates appropriately
         configured TLS connections. This is passed to C{startTLS} on the
@@ -362,7 +361,7 @@ class XMPPAuthenticator(xmlstream.ConnectAuthenticator):
     @type jid: L{jid.JID}
 
     @ivar password: password to be used during SASL authentication.
-    @type password: L{unicode}
+    @type password: L{str}
     """
 
     namespace = 'jabber:client'
diff --git a/src/twisted/words/protocols/jabber/component.py b/src/twisted/words/protocols/jabber/component.py
index 5563d909d..6ad0fd860 100644
--- a/src/twisted/words/protocols/jabber/component.py
+++ b/src/twisted/words/protocols/jabber/component.py
@@ -23,7 +23,6 @@ from zope.interface import implementer
 from twisted.application import service
 from twisted.internet import defer
 from twisted.python import log
-from twisted.python.compat import unicode
 from twisted.words.xish import domish
 from twisted.words.protocols.jabber import error, ijabber, jstrports, xmlstream
 from twisted.words.protocols.jabber.jid import internJID as JID
@@ -35,7 +34,7 @@ def componentFactory(componentid, password):
     XML stream factory for external server-side components.
 
     @param componentid: JID of the component.
-    @type componentid: L{unicode}
+    @type componentid: L{str}
     @param password: password used to authenticate to the server.
     @type password: C{str}
     """
@@ -59,7 +58,7 @@ class ComponentInitiatingInitializer(object):
         xs = self.xmlstream
         hs = domish.Element((self.xmlstream.namespace, "handshake"))
         digest = xmlstream.hashPassword(xs.sid, xs.authenticator.password)
-        hs.addContent(unicode(digest))
+        hs.addContent(str(digest))
 
         # Setup observer to watch for handshake result
         xs.addOnetimeObserver("/handshake", self._cbHandshake)
@@ -110,7 +109,7 @@ class ListenComponentAuthenticator(xmlstream.ListenAuthenticator):
     @since: 8.2
     @ivar secret: The shared secret used to authorized incoming component
                   connections.
-    @type secret: C{unicode}.
+    @type secret: C{str}.
     """
 
     namespace = NS_COMPONENT_ACCEPT
@@ -169,7 +168,7 @@ class ListenComponentAuthenticator(xmlstream.ListenAuthenticator):
         L{onHandshake}.
         """
         if (element.uri, element.name) == (self.namespace, 'handshake'):
-            self.onHandshake(unicode(element))
+            self.onHandshake(str(element))
         else:
             exc = error.StreamError('not-authorized')
             self.xmlstream.sendStreamError(exc)
@@ -185,7 +184,7 @@ class ListenComponentAuthenticator(xmlstream.ListenAuthenticator):
         be exchanged.
         """
         calculatedHash = xmlstream.hashPassword(self.xmlstream.sid,
-                                                unicode(self.secret))
+                                                str(self.secret))
         if handshake != calculatedHash:
             exc = error.StreamError('not-authorized', text='Invalid hash')
             self.xmlstream.sendStreamError(exc)
diff --git a/src/twisted/words/xish/domish.py b/src/twisted/words/xish/domish.py
index 04c705f8f..d94e96fdf 100644
--- a/src/twisted/words/xish/domish.py
+++ b/src/twisted/words/xish/domish.py
@@ -13,7 +13,6 @@ for use in streaming XML applications.
 
 from zope.interface import implementer, Interface, Attribute
 
-from twisted.python.compat import StringType, iteritems, itervalues
 from twisted.web import sux
 
 
@@ -69,7 +68,7 @@ class _ListSerializer:
             return
 
         # Shortcut, check to see if elem is actually a string (aka Cdata)
-        if isinstance(elem, StringType):
+        if isinstance(elem, str):
             write(escapeToXml(elem))
             return
 
@@ -78,7 +77,7 @@ class _ListSerializer:
         uri = elem.uri
         defaultUri, currentDefaultUri = elem.defaultUri, defaultUri
 
-        for p, u in iteritems(elem.localPrefixes):
+        for p, u in elem.localPrefixes.items():
             self.prefixes[u] = p
         self.prefixStack.append(list(elem.localPrefixes.keys()))
 
@@ -110,7 +109,7 @@ class _ListSerializer:
            (uri != defaultUri or not prefix or not inScope):
             write(" xmlns='%s'" % (defaultUri))
 
-        for p, u in iteritems(elem.localPrefixes):
+        for p, u in elem.localPrefixes.items():
             write(" xmlns:%s='%s'" % (p, u))
 
         # Serialize attributes
@@ -427,7 +426,7 @@ class Element(object):
         self.localPrefixes = localPrefixes or {}
         self.uri, self.name = qname
         if defaultUri is None and \
-           self.uri not in itervalues(self.localPrefixes):
+           self.uri not in self.localPrefixes.values():
             self.defaultUri = self.uri
         else:
             self.defaultUri = defaultUri
@@ -462,7 +461,7 @@ class Element(object):
         Retrieve the first CData (content) node
         """
         for n in self.children:
-            if isinstance(n, StringType):
+            if isinstance(n, str):
                 return n
         return ""

That would have been relatively easy to do.

rodrigc · 2020-07-24T22:12:58Z

So it looks like you chose to merge this PR into Twisted, and put my name as a reviewer without me actually
approving the review.

That's not very nice.

mthuurne · 2020-07-24T23:14:32Z

I re-read your comments yesterday and thought "You can do that in this PR, or do a separate one if you want." applied to all of them. Since I was pretty far with #1364 and I think that PR addresses all of the issues you raised, I thought it was safe to merge this one and move the discussion to the new PR. The scope of this PR is addressing the private definitions, while #1364 is for the public definitions.

I didn't want to offend you or your work as a reviewer. You're doing awesome work on Twisted and the issues you raise are valid improvements that I will address, just in a separate PR.

…ompat_private" This reverts commit 9a0c590, reversing changes made to 13e959d.

Revert #1363

mthuurne · 2020-07-25T09:37:50Z

I reverted the merge and created a new PR here: #1367.

rodrigc · 2020-07-27T00:29:35Z

@mthuurne it is quite clear that I did not transition this PR into the approved state in GitHub,
nor did I tag Trac ticket 9921 as approved. Twisted's development process is not perfect, but keeping logs
of review approvals before merging is important to the project.

These documents are a bit long, sometimes a bit outdated, and sometimes a little bit hard to follow,
but I suggest you re-read them to get a feel for the Twisted review process:

Looking at the audit trail of this PR, it is quite clear that you were very eager to merge this PR (which is OK to be
motivated and eager), and ignore my review feedback (which is not ideal, but sometimes difference of opinions exit). If you got another Twisted developer to approve this PR, over my objections, I would have been OK with that.

But the fact that you merged, put my name as Reviewer:, even though I did not fully approve the PR,
is just plain dishonest. Hopefully it was just a mistake on your part, but I don't fully believe this.

I'm going to stop reviewing your PR's.

There are other Twisted developers that you can work with to review your PR's, so you hopefully you can still move
forward and contribute to the project.

mthuurne · 2020-07-27T23:04:13Z

I won't deny that I was a bit frustrated that the review wasn't progressing. However, I did the sensible thing and took some rest. After I had some sleep, I re-read the entire ticket from the start and concluded that while you would prefer more changes to be made in this PR, that was optional rather than mandatory. From your reaction, it is now clear to me that was an incorrect assessment, but that's how I interpreted the situation at that time. So that's when I decided to merge PR 1363 and asked for your review of PR 1364, which continues the work of cleaning up uses of compat. I wouldn't have made that request if I didn't value your input.

Also I would like to stress again that I didn't ignore your review feedback: the removal of unicode, long etc. is addressed in PR 1364, of which I shared a draft version to demonstrate that the requested changes were in the pipeline. The updating of the types in docstrings is still on my to-do list, because I haven't figured out yet how to do that on a mass scale. Since there are many hundreds of incorrect docstrings at the moment, I don't think we can realistically fix this issue without at least some automation. This effort can also be reduced by removing @type and @rtype tags once pydoctor can read those from the annotations instead.

Regarding the development process, I should have waited for explicit approval instead of interpreting the situation on my own. But regarding the content, I think we are both working towards the same end result, which is Twisted with type annotations and correct documentation. I don't want one mistake on the process side to get in the way of reaching that goal.

rodrigc · 2020-07-27T23:44:06Z

Regarding the development process, I should have waited for explicit approval instead of interpreting the situation on my own.
Exactly.

glyph · 2020-07-30T06:13:31Z

But the fact that you merged, put my name as Reviewer:, even though I did not fully approve the PR,
is just plain dishonest. Hopefully it was just a mistake on your part, but I don't fully believe this.

I'm going to stop reviewing your PR's.

Craig, this really seems a bit harsh to me. It seems clear to me that Maarten was a bit overenthusiastic, but he also took it upon himself to do the revert when it was made clear that you had not intended to approve the review and he'd misinterpreted the process. As such it's hard for me to read malice or intentional subversion into his actions here.

Having everything in version control means we can address process-violation concerns here in retrospect as long as nobody's cut a release in the meanwhile.

rodrigc reviewed Jul 22, 2020

View reviewed changes

rodrigc suggested changes Jul 22, 2020

View reviewed changes

mthuurne force-pushed the 9921-mthuurne-cleanup_compat_private branch from 3c4b6e8 to bb6f8da Compare July 22, 2020 18:37

mthuurne mentioned this pull request Jul 23, 2020

Deprecate obsolete public definitions from twisted.python.compat #1364

Merged

5 tasks

mthuurne added 7 commits July 24, 2020 13:57

Remove _shouldEnableNewStyle() and _EXPECT_NEWSTYLE from compat

2df5236

Remove _PY35PLUS from compat

014e030

Remove _PY3 from compat

f40c6ac

Remove _tokenize() from compat

e7ab7be

Remove _keys() from compat

ad0c5b9

Remove _bytesChr() from compat

bc3d600

Remove _coercedUnicode() from compat

d93200c

mthuurne added 4 commits July 24, 2020 13:57

Remove _bytesRepr() from compat

5a1d439

Remove __b64decodebytes() and _b64encodebytes() from compat

df3aacd

Add empty news fragment

4484ac1

Remove test for domish.Element.addContent() raising TypeError

7ca09b0

We can now use type annotations to verify argument types.

mthuurne force-pushed the 9921-mthuurne-cleanup_compat_private branch from bb6f8da to 7ca09b0 Compare July 24, 2020 11:57

mthuurne merged commit 9a0c590 into trunk Jul 24, 2020

mthuurne deleted the 9921-mthuurne-cleanup_compat_private branch July 24, 2020 12:45

mthuurne added a commit that referenced this pull request Jul 25, 2020

Revert "Merge pull request #1363 from twisted/9921-mthuurne-cleanup_c…

18802b4

…ompat_private" This reverts commit 9a0c590, reversing changes made to 13e959d.

mthuurne mentioned this pull request Jul 25, 2020

Revert #1363 #1366

Merged

mthuurne added a commit that referenced this pull request Jul 25, 2020

Merge pull request #1366 from twisted/9921-mthuurne-revert

ce0de9c

Revert #1363

mthuurne mentioned this pull request Jul 25, 2020

Drop obsolete private definitions from twisted.python.compat #1367

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop obsolete private definitions from twisted.python.compat #1363

Drop obsolete private definitions from twisted.python.compat #1363

mthuurne commented Jul 22, 2020 •

edited

Loading

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc Jul 22, 2020

rodrigc left a comment

mthuurne commented Jul 22, 2020

rodrigc commented Jul 22, 2020

mthuurne commented Jul 22, 2020

rodrigc commented Jul 22, 2020

mthuurne commented Jul 22, 2020

rodrigc commented Jul 23, 2020

mthuurne commented Jul 23, 2020 •

edited

Loading

rodrigc commented Jul 23, 2020

mthuurne commented Jul 23, 2020 •

edited

Loading

rodrigc commented Jul 24, 2020

rodrigc commented Jul 24, 2020

mthuurne commented Jul 24, 2020

mthuurne commented Jul 25, 2020

rodrigc commented Jul 27, 2020

mthuurne commented Jul 27, 2020

rodrigc commented Jul 27, 2020

glyph commented Jul 30, 2020

Drop obsolete private definitions from twisted.python.compat #1363

Drop obsolete private definitions from twisted.python.compat #1363

Conversation

mthuurne commented Jul 22, 2020 • edited Loading

Contributor Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigc left a comment

Choose a reason for hiding this comment

mthuurne commented Jul 22, 2020

rodrigc commented Jul 22, 2020

mthuurne commented Jul 22, 2020

rodrigc commented Jul 22, 2020

mthuurne commented Jul 22, 2020

rodrigc commented Jul 23, 2020

mthuurne commented Jul 23, 2020 • edited Loading

rodrigc commented Jul 23, 2020

mthuurne commented Jul 23, 2020 • edited Loading

rodrigc commented Jul 24, 2020

rodrigc commented Jul 24, 2020

mthuurne commented Jul 24, 2020

mthuurne commented Jul 25, 2020

rodrigc commented Jul 27, 2020

mthuurne commented Jul 27, 2020

rodrigc commented Jul 27, 2020

glyph commented Jul 30, 2020

mthuurne commented Jul 22, 2020 •

edited

Loading

mthuurne commented Jul 23, 2020 •

edited

Loading

mthuurne commented Jul 23, 2020 •

edited

Loading