Correct/clarify message for wrong-import-order

doc/data/messages/i/implicit-str-concat/details.rst

@@ -11,7 +11,7 @@ In order to detect this case, you must enable `check-str-concat-over-line-jumps`
 .. code-block:: toml
 
     [STRING_CONSTANT]
-    check-str-concat-over-line-jumps = yes
+    check-str-concat-over-line-jumps = true
 
 However, the drawback of this setting is that it will trigger false positive
 for string parameters passed on multiple lines in function calls:

doc/whatsnew/fragments/8808.bugfix

@@ -0,0 +1,8 @@
+Improve the message provided for wrong-import-order check.  Instead of the import statement ("import x"), the message now specifies the import that is out of order and which imports should come after it.  As reported in the issue, this is particularly helpful if there are multiple imports on a single line that do not follow the PEP8 convention.
+
+The message will report imports as follows:
+For "import X", it will report "(standard/third party/first party/local) import X"
+For "import X.Y" and "from X import Y", it will report "(standard/third party/first party/local) import X.Y"
+The import category is specified to provide explanation as to why pylint has issued the message and guidence to the developer on how to fix the problem.
+
+Closes #8808

pylint/checkers/imports.py

@@ -28,6 +28,7 @@
     is_sys_guard,
     node_ignores_exception,
 )
+from pylint.constants import MAX_NUMBER_OF_IMPORT_SHOWN
 from pylint.exceptions import EmptyReportError
 from pylint.graph import DotBackend, get_cycles
 from pylint.interfaces import HIGH
@@ -781,6 +782,7 @@ def _check_imports_order(
             )
             import_category = isort_driver.place_module(package)
             node_and_package_import = (node, package)
+
             if import_category in {"FUTURE", "STDLIB"}:
                 std_imports.append(node_and_package_import)
                 wrong_import = (
@@ -794,9 +796,13 @@ def _check_imports_order(
                     self.add_message(
                         "wrong-import-order",
                         node=node,
-                        args=(
-                            f'standard import "{node.as_string()}"',
-                            f'"{wrong_import[0][0].as_string()}"',
+                        args=(  ## TODO - this isn't right for multiple on the same line...
+                            f'standard import "{self._get_full_import_name((node, package))}"',
+                            self._get_out_of_order_string(
+                                third_party_not_ignored,
+                                first_party_not_ignored,
+                                local_not_ignored,
+                            ),
                         ),
                     )
             elif import_category == "THIRDPARTY":
@@ -815,8 +821,10 @@ def _check_imports_order(
                         "wrong-import-order",
                         node=node,
                         args=(
-                            f'third party import "{node.as_string()}"',
-                            f'"{wrong_import[0][0].as_string()}"',
+                            f'third party import "{self._get_full_import_name((node, package))}"',
+                            self._get_out_of_order_string(
+                                None, first_party_not_ignored, local_not_ignored
+                            ),
                         ),
                     )
             elif import_category == "FIRSTPARTY":
@@ -835,8 +843,10 @@ def _check_imports_order(
                         "wrong-import-order",
                         node=node,
                         args=(
-                            f'first party import "{node.as_string()}"',
-                            f'"{wrong_import[0][0].as_string()}"',
+                            f'first party import "{self._get_full_import_name((node, package))}"',
+                            self._get_out_of_order_string(
+                                None, None, local_not_ignored
+                            ),
                         ),
                     )
             elif import_category == "LOCALFOLDER":
@@ -850,6 +860,157 @@ def _check_imports_order(
                         )
         return std_imports, external_imports, local_imports
 
+    def _get_out_of_order_string(
+        self,
+        third_party_imports: list[tuple[ImportNode, str]] | None,
+        first_party_imports: list[tuple[ImportNode, str]] | None,
+        local_imports: list[tuple[ImportNode, str]] | None,
+    ) -> str:
+        # construct the string listing out of order imports used in the message
+        # for wrong-import-order
+        if third_party_imports:
+            plural = "s" if len(third_party_imports) > 1 else ""
+            if len(third_party_imports) > MAX_NUMBER_OF_IMPORT_SHOWN:
+                imports_list = (
+                    ", ".join(
+                        [
+                            f'"{self._get_full_import_name(tpi)}"'
+                            for tpi in third_party_imports[
+                                : int(MAX_NUMBER_OF_IMPORT_SHOWN // 2)
+                            ]
+                        ]
+                    )
+                    + " (...) "
+                    + ", ".join(
+                        [
+                            f'"{self._get_full_import_name(tpi)}"'
+                            for tpi in third_party_imports[
+                                int(-MAX_NUMBER_OF_IMPORT_SHOWN // 2) :
+                            ]
+                        ]
+                    )
+                )
+            else:
+                imports_list = ", ".join(
+                    [
+                        f'"{self._get_full_import_name(tpi)}"'
+                        for tpi in third_party_imports
+                    ]
+                )
+            third_party = f"third party import{plural} {imports_list}"
+        else:
+            third_party = ""
+
+        if first_party_imports:
+            plural = "s" if len(first_party_imports) > 1 else ""
+            if len(first_party_imports) > MAX_NUMBER_OF_IMPORT_SHOWN:
+                imports_list = (
+                    ", ".join(
+                        [
+                            f'"{self._get_full_import_name(tpi)}"'
+                            for tpi in first_party_imports[
+                                : int(MAX_NUMBER_OF_IMPORT_SHOWN // 2)
+                            ]
+                        ]
+                    )
+                    + " (...) "
+                    + ", ".join(
+                        [
+                            f'"{self._get_full_import_name(tpi)}"'
+                            for tpi in first_party_imports[
+                                int(-MAX_NUMBER_OF_IMPORT_SHOWN // 2) :
+                            ]
+                        ]
+                    )
+                )
+            else:
+                imports_list = ", ".join(
+                    [
+                        f'"{self._get_full_import_name(fpi)}"'
+                        for fpi in first_party_imports
+                    ]
+                )
+            first_party = f"first party import{plural} {imports_list}"
+        else:
+            first_party = ""
+
+        if local_imports:
+            plural = "s" if len(local_imports) > 1 else ""
+            if len(local_imports) > MAX_NUMBER_OF_IMPORT_SHOWN:
+                imports_list = (
+                    ", ".join(
+                        [
+                            f'"{self._get_full_import_name(tpi)}"'
+                            for tpi in local_imports[
+                                : int(MAX_NUMBER_OF_IMPORT_SHOWN // 2)
+                            ]
+                        ]
+                    )
+                    + " (...) "
+                    + ", ".join(
+                        [
+                            f'"{self._get_full_import_name(tpi)}"'
+                            for tpi in local_imports[
+                                int(-MAX_NUMBER_OF_IMPORT_SHOWN // 2) :
+                            ]
+                        ]
+                    )
+                )
+            else:
+                imports_list = ", ".join(
+                    [f'"{self._get_full_import_name(li)}"' for li in local_imports]
+                )
+            local = f"local import{plural} {imports_list}"
+        else:
+            local = ""
+
+        delimiter_third_party = (
+            (
+                ", "
+                if (first_party and local)
+                else (" and " if (first_party or local) else "")
+            )
+            if third_party
+            else ""
+        )
+        delimiter_first_party1 = (
+            (", " if (third_party and local) else " ") if first_party else ""
+        )
+        delimiter_first_party2 = ("and " if local else "") if first_party else ""
+        delimiter_first_party = f"{delimiter_first_party1}{delimiter_first_party2}"
+        msg = (
+            f"{third_party}{delimiter_third_party}"
+            f"{first_party}{delimiter_first_party}"
+            f'{local if local else ""}'
+        )
+
+        return msg
+
+    def _get_full_import_name(self, importNode: ImportNode) -> str:
+        # construct a more descriptive name of the import
+        # for: import X, this returns X
+        # for: import X.Y this returns X.Y
+        # for: from X import Y, this returns X.Y
+
+        try:
+            # this will only succeed for ImportFrom nodes, which in themselves
+            # contain the information needed to reconstruct the package
+            return f"{importNode[0].modname}.{importNode[0].names[0][0]}"
+        except AttributeError:
+            # in all other cases, the import will either be X or X.Y
+            node: str = importNode[0].names[0][0]
+            package: str = importNode[1]
+
+            if node.split(".")[0] == package:
+                # this is sufficient with one import per line, since package = X
+                # and node = X.Y or X
+                return node
+
+            # when there is a node that contains multiple imports, the "current"
+            # import being analyzed is specified by package (node is the first
+            # import on the line and therefore != package in this case)
+            return package
+
     def _get_imported_module(
         self, importnode: ImportNode, modname: str
     ) -> nodes.Module | None:

pylint/constants.py

@@ -274,3 +274,5 @@ def _get_pylint_home() -> str:
     "__ixor__",
     "__ior__",
 ]
+
+MAX_NUMBER_OF_IMPORT_SHOWN = 6

tests/functional/w/wrong_import_order.py

@@ -1,5 +1,5 @@
 """Checks import order rule"""
-# pylint: disable=unused-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
+# pylint: disable=unused-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level,multiple-imports,reimported
 from __future__ import absolute_import
 try:
     from six.moves import configparser
@@ -19,10 +19,20 @@
 from . import package
 import astroid  # [wrong-import-order]
 from . import package2
+import pylint.checkers  # [wrong-import-order]
+from pylint import config  # [wrong-import-order]
+import pylint.sys  # [wrong-import-order]
+from pylint import pyreverse  # [wrong-import-order]
 from .package2 import Class2
 from ..package3 import Class3
+from . import package4
+from .package4  import Class4
 from six.moves.urllib.parse import quote # [wrong-import-order]
-
+import pylint.constants # [wrong-import-order]
+import re, requests # [wrong-import-order, wrong-import-order]
+import pylint.exceptions  # [wrong-import-order]
+import pylint.message  # [wrong-import-order]
+import time  # [wrong-import-order]
 
 LOGGER = logging.getLogger(__name__)
 

tests/functional/w/wrong_import_order.txt

@@ -1,6 +1,16 @@
-wrong-import-order:12:0:12:14::"standard import ""import os.path"" should be placed before ""import six""":UNDEFINED
-wrong-import-order:14:0:14:10::"standard import ""import sys"" should be placed before ""import six""":UNDEFINED
-wrong-import-order:15:0:15:15::"standard import ""import datetime"" should be placed before ""import six""":UNDEFINED
-wrong-import-order:18:0:18:22::"third party import ""import totally_missing"" should be placed before ""from .package import Class""":UNDEFINED
-wrong-import-order:20:0:20:14::"third party import ""import astroid"" should be placed before ""from .package import Class""":UNDEFINED
-wrong-import-order:24:0:24:40::"third party import ""from six.moves.urllib.parse import quote"" should be placed before ""from .package import Class""":UNDEFINED
+wrong-import-order:12:0:12:14::"standard import ""os.path"" should be placed before third party import ""six""":UNDEFINED
+wrong-import-order:14:0:14:10::"standard import ""sys"" should be placed before third party imports ""six"", ""astroid.are_exclusive""":UNDEFINED
+wrong-import-order:15:0:15:15::"standard import ""datetime"" should be placed before third party imports ""six"", ""astroid.are_exclusive""":UNDEFINED
+wrong-import-order:18:0:18:22::"third party import ""totally_missing"" should be placed before local import ""package.Class""":UNDEFINED
+wrong-import-order:20:0:20:14::"third party import ""astroid"" should be placed before local imports ""package.Class"", "".package""":UNDEFINED
+wrong-import-order:22:0:22:22::"first party import ""pylint.checkers"" should be placed before local imports ""package.Class"", "".package"", "".package2""":UNDEFINED
+wrong-import-order:23:0:23:25::"first party import ""pylint.config"" should be placed before local imports ""package.Class"", "".package"", "".package2""":UNDEFINED
+wrong-import-order:24:0:24:17::"first party import ""pylint.sys"" should be placed before local imports ""package.Class"", "".package"", "".package2""":UNDEFINED
+wrong-import-order:25:0:25:28::"first party import ""pylint.pyreverse"" should be placed before local imports ""package.Class"", "".package"", "".package2""":UNDEFINED
+wrong-import-order:30:0:30:40::"third party import ""six.moves.urllib.parse.quote"" should be placed before first party imports ""pylint.checkers"", ""pylint.config"", ""pylint.sys"", ""pylint.pyreverse"" and local imports ""package.Class"", "".package"", "".package2"" (...) ""package3.Class3"", "".package4"", ""package4.Class4""":UNDEFINED
+wrong-import-order:31:0:31:23::"first party import ""pylint.constants"" should be placed before local imports ""package.Class"", "".package"", "".package2"" (...) ""package3.Class3"", "".package4"", ""package4.Class4""":UNDEFINED
+wrong-import-order:32:0:32:19::"standard import ""re"" should be placed before third party imports ""six"", ""astroid.are_exclusive"", ""unused_import"", ""totally_missing"", ""astroid"", ""six.moves.urllib.parse.quote"", first party imports ""pylint.checkers"", ""pylint.config"", ""pylint.sys"", ""pylint.pyreverse"", ""pylint.constants"", and local imports ""package.Class"", "".package"", "".package2"" (...) ""package3.Class3"", "".package4"", ""package4.Class4""":UNDEFINED
+wrong-import-order:32:0:32:19::"third party import ""requests"" should be placed before first party imports ""pylint.checkers"", ""pylint.config"", ""pylint.sys"", ""pylint.pyreverse"", ""pylint.constants"" and local imports ""package.Class"", "".package"", "".package2"" (...) ""package3.Class3"", "".package4"", ""package4.Class4""":UNDEFINED
+wrong-import-order:33:0:33:24::"first party import ""pylint.exceptions"" should be placed before local imports ""package.Class"", "".package"", "".package2"" (...) ""package3.Class3"", "".package4"", ""package4.Class4""":UNDEFINED
+wrong-import-order:34:0:34:21::"first party import ""pylint.message"" should be placed before local imports ""package.Class"", "".package"", "".package2"" (...) ""package3.Class3"", "".package4"", ""package4.Class4""":UNDEFINED
+wrong-import-order:35:0:35:11::"standard import ""time"" should be placed before third party imports ""six"", ""astroid.are_exclusive"", ""unused_import"" (...) ""astroid"", ""six.moves.urllib.parse.quote"", ""requests"", first party imports ""pylint.checkers"", ""pylint.config"", ""pylint.sys"" (...) ""pylint.constants"", ""pylint.exceptions"", ""pylint.message"", and local imports ""package.Class"", "".package"", "".package2"" (...) ""package3.Class3"", "".package4"", ""package4.Class4""":UNDEFINED