[#9634] Add an isort configuration

[hawkowl]

https://twistedmatrix.com/trac/ticket/9634#ticket

[hawkowl]

an example from _sslverify:

from __future__ import absolute_import, division

import warnings
from hashlib import md5

from zope.interface import Interface, implementer

from OpenSSL import SSL, crypto
from OpenSSL._util import lib as pyOpenSSLlib

from constantly import FlagConstant, Flags, NamedConstant, Names
from incremental import Version

from twisted.internet.abstract import isIPAddress, isIPv6Address
from twisted.internet.defer import Deferred
from twisted.internet.error import CertificateError, VerifyError
from twisted.internet.interfaces import (
    IAcceptableCiphers,
    ICipher,
    IOpenSSLClientConnectionCreator,
    IOpenSSLContextFactory,
)
from twisted.python import log, util
from twisted.python._oldstyle import _oldStyle
from twisted.python.compat import nativeString, unicode
from twisted.python.deprecate import _mutuallyExclusiveArguments, deprecated
from twisted.python.failure import Failure
from twisted.python.randbytes import secureRandom
from twisted.python.util import FancyEqMixin

[twm]

❤️ isort, it's great.

I ran it recursively and skimmed the (enormous) diff. Most changes seemed reasonable enough.

I will note that isort can't be applied blindly:

• I saw some test failures when I did so.
• The resulting whitespace doesn't necessarily pass twistedchecker.

We should still merge this, though. It gets all isort-using developers on the same page. isort is useful even if we don't lint on it.

[twm]

Should twisted be in localfolder? Existing files are not consistent about this, e.g. when I run isort with this config on trunk this file's imports are broken apart:

--- a/src/twisted/application/runner/test/test_exit.py
+++ b/src/twisted/application/runner/test/test_exit.py
@@ -5,12 +5,11 @@
 Tests for L{twisted.application.runner._exit}.
 """
 
-from twisted.python.compat import NativeStringIO
-from ...runner import _exit
-from .._exit import exit, ExitStatus
-
 import twisted.trial.unittest
+from twisted.python.compat import NativeStringIO
 
+from ...runner import _exit
+from .._exit import ExitStatus, exit
 
 
 class ExitTests(twisted.trial.unittest.TestCase):

vs. this file, where they were always separate:

--- a/src/twisted/application/runner/test/test_pidfile.py
+++ b/src/twisted/application/runner/test/test_pidfile.py
@@ -5,27 +5,30 @@
 Tests for L{twisted.application.runner._pidfile}.
 """
 
-from functools import wraps
 import errno
-from os import getpid, name as SYSTEM_NAME
+from functools import wraps
 from io import BytesIO
+from os import getpid, name as SYSTEM_NAME
 
 from zope.interface import implementer
 from zope.interface.verify import verifyObject
 
+import twisted.trial.unittest
 from twisted.python.filepath import IFilePath
 from twisted.python.runtime import platform
+from twisted.trial.unittest import SkipTest
 
 from ...runner import _pidfile
 from .._pidfile import (
-    IPIDFile, PIDFile, NonePIDFile,
-    AlreadyRunningError, InvalidPIDFileError, StalePIDFileError,
+    AlreadyRunningError,
+    InvalidPIDFileError,
+    IPIDFile,
+    NonePIDFile,
     NoPIDFound,
+    PIDFile,
+    StalePIDFileError,
 )
 
-import twisted.trial.unittest
-from twisted.trial.unittest import SkipTest
-
 
 def ifPlatformSupported(f):
     """

Either seems okay to me. We really shouldn't mix these styles in the same file, after all.

[glyph]

I will note that isort can't be applied blindly:

I saw some test failures when I did so.

This is really concerning to me and suggests that there is something incorrect about the configuration. Before landing this, could we perhaps address the test failures with some exclusions so it can be applied blindly?

[wsanchez]

I would suggest putting this config into pyproject.toml instead…

[twm]

This is really concerning to me and suggests that there is something incorrect about the configuration. Before landing this, could we perhaps address the test failures with some exclusions so it can be applied blindly?

I don't think that it suggests anything about the configuration. It suggests that there are import cycles or ordering dependencies within Twisted. This isn't new information — we've seen a trickle of such issues, like #8267 and #7096.

I do not think that we should expand the scope of this ticket to include fixing these issues. The ticket says "add an isort config" and this PR does that. It is a useful improvement that will improve the developer experience. It has merit on those terms and should be merged.

[hawkowl]

This PR produces working output when applied across the board, with the following caveats:

• twistedchecker gets mad because there's inconsistent newlines, and sometimes excess ones because of how the moving around of imports works (fixed by something like black)
• it requires ignoring a few files, and moving t.p.compat to the top because it sometimes has side-effects by patching socket (lol)

You can see the test runs and the diff at #1139.

[twm]

Thanks for picking this up again!

it requires ignoring a few files, and moving t.p.compat to the top because it sometimes has side-effects by patching socket (lol)

Perhaps we should add an import of twisted.python.compat to src/twisted/__init__.py so that these patches are consistently applied regardless of which module user code imports? (Not that I think that need be in this PR.)

[twm]

Thank you for investigating and fixing this!