From 10867b5d8e0124b28a01d97ac415aa85e1eda9a1 Mon Sep 17 00:00:00 2001 From: azinneck0485 <123660683+azinneck0485@users.noreply.github.com> Date: Wed, 8 Nov 2023 00:01:25 -0500 Subject: [PATCH 1/2] Correct/clarify message for wrong-import-order (#9236) 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 Co-authored-by: Pierre Sassoulas --- doc/whatsnew/fragments/8808.bugfix | 8 + pylint/checkers/imports.py | 175 +++++++++++++++++++++- pylint/constants.py | 2 + tests/functional/w/wrong_import_order.py | 14 +- tests/functional/w/wrong_import_order.txt | 22 ++- 5 files changed, 206 insertions(+), 15 deletions(-) create mode 100644 doc/whatsnew/fragments/8808.bugfix diff --git a/doc/whatsnew/fragments/8808.bugfix b/doc/whatsnew/fragments/8808.bugfix new file mode 100644 index 0000000000..a26d7358a0 --- /dev/null +++ b/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 diff --git a/pylint/checkers/imports.py b/pylint/checkers/imports.py index 82ea529cb9..cca0f58c9a 100644 --- a/pylint/checkers/imports.py +++ b/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: diff --git a/pylint/constants.py b/pylint/constants.py index 7956d70c9d..e51022e654 100644 --- a/pylint/constants.py +++ b/pylint/constants.py @@ -274,3 +274,5 @@ def _get_pylint_home() -> str: "__ixor__", "__ior__", ] + +MAX_NUMBER_OF_IMPORT_SHOWN = 6 diff --git a/tests/functional/w/wrong_import_order.py b/tests/functional/w/wrong_import_order.py index ed0615b0c0..b838d6e9da 100644 --- a/tests/functional/w/wrong_import_order.py +++ b/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__) diff --git a/tests/functional/w/wrong_import_order.txt b/tests/functional/w/wrong_import_order.txt index c0706a9d27..068d2140d8 100644 --- a/tests/functional/w/wrong_import_order.txt +++ b/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 From 8bff674747e8e3a2a80744528a0259099acb1afc Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sun, 26 Nov 2023 11:31:16 +0100 Subject: [PATCH 2/2] Fix doc generation in implicit-str-concat New sphinx now check that a block of code is valid. --- doc/data/messages/i/implicit-str-concat/details.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/data/messages/i/implicit-str-concat/details.rst b/doc/data/messages/i/implicit-str-concat/details.rst index 07b9fc172c..6b3f2c32f5 100644 --- a/doc/data/messages/i/implicit-str-concat/details.rst +++ b/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: