Skip to content

Commit c4ffae1

Browse files
azinneck0485Pierre-Sassoulas
azinneck0485
and
Pierre-Sassoulas
committed
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 <[email protected]>
1 parent 6f83d5a commit c4ffae1

File tree

5 files changed

+206
-15
lines changed

5 files changed

+206
-15
lines changed

‎doc/whatsnew/fragments/8808.bugfix

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
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.
2+
3+
The message will report imports as follows:
4+
For "import X", it will report "(standard/third party/first party/local) import X"
5+
For "import X.Y" and "from X import Y", it will report "(standard/third party/first party/local) import X.Y"
6+
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.
7+
8+
Closes #8808

‎pylint/checkers/imports.py

Lines changed: 168 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
is_sys_guard,
2929
node_ignores_exception,
3030
)
31+
from pylint.constants import MAX_NUMBER_OF_IMPORT_SHOWN
3132
from pylint.exceptions import EmptyReportError
3233
from pylint.graph import DotBackend, get_cycles
3334
from pylint.interfaces import HIGH
@@ -781,6 +782,7 @@ def _check_imports_order(
781782
)
782783
import_category = isort_driver.place_module(package)
783784
node_and_package_import = (node, package)
785+
784786
if import_category in {"FUTURE", "STDLIB"}:
785787
std_imports.append(node_and_package_import)
786788
wrong_import = (
@@ -794,9 +796,13 @@ def _check_imports_order(
794796
self.add_message(
795797
"wrong-import-order",
796798
node=node,
797-
args=(
798-
f'standard import "{node.as_string()}"',
799-
f'"{wrong_import[0][0].as_string()}"',
799+
args=( ## TODO - this isn't right for multiple on the same line...
800+
f'standard import "{self._get_full_import_name((node, package))}"',
801+
self._get_out_of_order_string(
802+
third_party_not_ignored,
803+
first_party_not_ignored,
804+
local_not_ignored,
805+
),
800806
),
801807
)
802808
elif import_category == "THIRDPARTY":
@@ -815,8 +821,10 @@ def _check_imports_order(
815821
"wrong-import-order",
816822
node=node,
817823
args=(
818-
f'third party import "{node.as_string()}"',
819-
f'"{wrong_import[0][0].as_string()}"',
824+
f'third party import "{self._get_full_import_name((node, package))}"',
825+
self._get_out_of_order_string(
826+
None, first_party_not_ignored, local_not_ignored
827+
),
820828
),
821829
)
822830
elif import_category == "FIRSTPARTY":
@@ -835,8 +843,10 @@ def _check_imports_order(
835843
"wrong-import-order",
836844
node=node,
837845
args=(
838-
f'first party import "{node.as_string()}"',
839-
f'"{wrong_import[0][0].as_string()}"',
846+
f'first party import "{self._get_full_import_name((node, package))}"',
847+
self._get_out_of_order_string(
848+
None, None, local_not_ignored
849+
),
840850
),
841851
)
842852
elif import_category == "LOCALFOLDER":
@@ -850,6 +860,157 @@ def _check_imports_order(
850860
)
851861
return std_imports, external_imports, local_imports
852862

863+
def _get_out_of_order_string(
864+
self,
865+
third_party_imports: list[tuple[ImportNode, str]] | None,
866+
first_party_imports: list[tuple[ImportNode, str]] | None,
867+
local_imports: list[tuple[ImportNode, str]] | None,
868+
) -> str:
869+
# construct the string listing out of order imports used in the message
870+
# for wrong-import-order
871+
if third_party_imports:
872+
plural = "s" if len(third_party_imports) > 1 else ""
873+
if len(third_party_imports) > MAX_NUMBER_OF_IMPORT_SHOWN:
874+
imports_list = (
875+
", ".join(
876+
[
877+
f'"{self._get_full_import_name(tpi)}"'
878+
for tpi in third_party_imports[
879+
: int(MAX_NUMBER_OF_IMPORT_SHOWN // 2)
880+
]
881+
]
882+
)
883+
+ " (...) "
884+
+ ", ".join(
885+
[
886+
f'"{self._get_full_import_name(tpi)}"'
887+
for tpi in third_party_imports[
888+
int(-MAX_NUMBER_OF_IMPORT_SHOWN // 2) :
889+
]
890+
]
891+
)
892+
)
893+
else:
894+
imports_list = ", ".join(
895+
[
896+
f'"{self._get_full_import_name(tpi)}"'
897+
for tpi in third_party_imports
898+
]
899+
)
900+
third_party = f"third party import{plural} {imports_list}"
901+
else:
902+
third_party = ""
903+
904+
if first_party_imports:
905+
plural = "s" if len(first_party_imports) > 1 else ""
906+
if len(first_party_imports) > MAX_NUMBER_OF_IMPORT_SHOWN:
907+
imports_list = (
908+
", ".join(
909+
[
910+
f'"{self._get_full_import_name(tpi)}"'
911+
for tpi in first_party_imports[
912+
: int(MAX_NUMBER_OF_IMPORT_SHOWN // 2)
913+
]
914+
]
915+
)
916+
+ " (...) "
917+
+ ", ".join(
918+
[
919+
f'"{self._get_full_import_name(tpi)}"'
920+
for tpi in first_party_imports[
921+
int(-MAX_NUMBER_OF_IMPORT_SHOWN // 2) :
922+
]
923+
]
924+
)
925+
)
926+
else:
927+
imports_list = ", ".join(
928+
[
929+
f'"{self._get_full_import_name(fpi)}"'
930+
for fpi in first_party_imports
931+
]
932+
)
933+
first_party = f"first party import{plural} {imports_list}"
934+
else:
935+
first_party = ""
936+
937+
if local_imports:
938+
plural = "s" if len(local_imports) > 1 else ""
939+
if len(local_imports) > MAX_NUMBER_OF_IMPORT_SHOWN:
940+
imports_list = (
941+
", ".join(
942+
[
943+
f'"{self._get_full_import_name(tpi)}"'
944+
for tpi in local_imports[
945+
: int(MAX_NUMBER_OF_IMPORT_SHOWN // 2)
946+
]
947+
]
948+
)
949+
+ " (...) "
950+
+ ", ".join(
951+
[
952+
f'"{self._get_full_import_name(tpi)}"'
953+
for tpi in local_imports[
954+
int(-MAX_NUMBER_OF_IMPORT_SHOWN // 2) :
955+
]
956+
]
957+
)
958+
)
959+
else:
960+
imports_list = ", ".join(
961+
[f'"{self._get_full_import_name(li)}"' for li in local_imports]
962+
)
963+
local = f"local import{plural} {imports_list}"
964+
else:
965+
local = ""
966+
967+
delimiter_third_party = (
968+
(
969+
", "
970+
if (first_party and local)
971+
else (" and " if (first_party or local) else "")
972+
)
973+
if third_party
974+
else ""
975+
)
976+
delimiter_first_party1 = (
977+
(", " if (third_party and local) else " ") if first_party else ""
978+
)
979+
delimiter_first_party2 = ("and " if local else "") if first_party else ""
980+
delimiter_first_party = f"{delimiter_first_party1}{delimiter_first_party2}"
981+
msg = (
982+
f"{third_party}{delimiter_third_party}"
983+
f"{first_party}{delimiter_first_party}"
984+
f'{local if local else ""}'
985+
)
986+
987+
return msg
988+
989+
def _get_full_import_name(self, importNode: ImportNode) -> str:
990+
# construct a more descriptive name of the import
991+
# for: import X, this returns X
992+
# for: import X.Y this returns X.Y
993+
# for: from X import Y, this returns X.Y
994+
995+
try:
996+
# this will only succeed for ImportFrom nodes, which in themselves
997+
# contain the information needed to reconstruct the package
998+
return f"{importNode[0].modname}.{importNode[0].names[0][0]}"
999+
except AttributeError:
1000+
# in all other cases, the import will either be X or X.Y
1001+
node: str = importNode[0].names[0][0]
1002+
package: str = importNode[1]
1003+
1004+
if node.split(".")[0] == package:
1005+
# this is sufficient with one import per line, since package = X
1006+
# and node = X.Y or X
1007+
return node
1008+
1009+
# when there is a node that contains multiple imports, the "current"
1010+
# import being analyzed is specified by package (node is the first
1011+
# import on the line and therefore != package in this case)
1012+
return package
1013+
8531014
def _get_imported_module(
8541015
self, importnode: ImportNode, modname: str
8551016
) -> nodes.Module | None:

‎pylint/constants.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,3 +274,5 @@ def _get_pylint_home() -> str:
274274
"__ixor__",
275275
"__ior__",
276276
]
277+
278+
MAX_NUMBER_OF_IMPORT_SHOWN = 6

‎tests/functional/w/wrong_import_order.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
"""Checks import order rule"""
2-
# pylint: disable=unused-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level
2+
# pylint: disable=unused-import,ungrouped-imports,import-error,no-name-in-module,relative-beyond-top-level,multiple-imports,reimported
33
from __future__ import absolute_import
44
try:
55
from six.moves import configparser
@@ -19,10 +19,20 @@
1919
from . import package
2020
import astroid # [wrong-import-order]
2121
from . import package2
22+
import pylint.checkers # [wrong-import-order]
23+
from pylint import config # [wrong-import-order]
24+
import pylint.sys # [wrong-import-order]
25+
from pylint import pyreverse # [wrong-import-order]
2226
from .package2 import Class2
2327
from ..package3 import Class3
28+
from . import package4
29+
from .package4 import Class4
2430
from six.moves.urllib.parse import quote # [wrong-import-order]
25-
31+
import pylint.constants # [wrong-import-order]
32+
import re, requests # [wrong-import-order, wrong-import-order]
33+
import pylint.exceptions # [wrong-import-order]
34+
import pylint.message # [wrong-import-order]
35+
import time # [wrong-import-order]
2636

2737
LOGGER = logging.getLogger(__name__)
2838

‎tests/functional/w/wrong_import_order.txt

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,16 @@
1-
wrong-import-order:12:0:12:14::"standard import ""import os.path"" should be placed before ""import six""":UNDEFINED
2-
wrong-import-order:14:0:14:10::"standard import ""import sys"" should be placed before ""import six""":UNDEFINED
3-
wrong-import-order:15:0:15:15::"standard import ""import datetime"" should be placed before ""import six""":UNDEFINED
4-
wrong-import-order:18:0:18:22::"third party import ""import totally_missing"" should be placed before ""from .package import Class""":UNDEFINED
5-
wrong-import-order:20:0:20:14::"third party import ""import astroid"" should be placed before ""from .package import Class""":UNDEFINED
6-
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
1+
wrong-import-order:12:0:12:14::"standard import ""os.path"" should be placed before third party import ""six""":UNDEFINED
2+
wrong-import-order:14:0:14:10::"standard import ""sys"" should be placed before third party imports ""six"", ""astroid.are_exclusive""":UNDEFINED
3+
wrong-import-order:15:0:15:15::"standard import ""datetime"" should be placed before third party imports ""six"", ""astroid.are_exclusive""":UNDEFINED
4+
wrong-import-order:18:0:18:22::"third party import ""totally_missing"" should be placed before local import ""package.Class""":UNDEFINED
5+
wrong-import-order:20:0:20:14::"third party import ""astroid"" should be placed before local imports ""package.Class"", "".package""":UNDEFINED
6+
wrong-import-order:22:0:22:22::"first party import ""pylint.checkers"" should be placed before local imports ""package.Class"", "".package"", "".package2""":UNDEFINED
7+
wrong-import-order:23:0:23:25::"first party import ""pylint.config"" should be placed before local imports ""package.Class"", "".package"", "".package2""":UNDEFINED
8+
wrong-import-order:24:0:24:17::"first party import ""pylint.sys"" should be placed before local imports ""package.Class"", "".package"", "".package2""":UNDEFINED
9+
wrong-import-order:25:0:25:28::"first party import ""pylint.pyreverse"" should be placed before local imports ""package.Class"", "".package"", "".package2""":UNDEFINED
10+
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
11+
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
12+
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
13+
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
14+
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
15+
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
16+
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

0 commit comments

Comments
 (0)