Skip to content

fix: scope wrong-import-position pragma to specific line only #10590

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+39 −6
Open

fix: scope wrong-import-position pragma to specific line only #10590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8cace41
fix: scope wrong-import-position pragma to specific line only
emmanuel-ferdman Sep 25, 2025
a7e937a
fix: scope wrong-import-position pragma to specific line only
emmanuel-ferdman Sep 25, 2025
f957d35
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/10589.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Scope ``wrong-import-position`` pragma to specific line only.

Closes #10589
4 changes: 1 addition & 3 deletions pylint/checkers/imports.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,9 +702,7 @@ def _check_position(self, node: ImportNode) -> None:
# if a first non-import instruction has already been encountered,
# it means the import comes after it and therefore is not well placed
if self._first_non_import_node:
if self.linter.is_message_enabled(
"wrong-import-position", self._first_non_import_node.fromlineno
):
if self.linter.is_message_enabled("wrong-import-position", node.fromlineno):
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instinctively I would say the fix need to be a lot more complex than that. If one line is disabled it means not only that the message shouldn't be raised for it but also that other line should not raise if the only issue for them it the disabled line.

i.e. the expected is

import a
import b
import c
import b  # [wrong-import-position]
import a  # [wrong-import-position]
import c 
import b  # pylint: disable=wrong-import-position
import a  
import c  
import b  # pylint: disable=wrong-import-position
import c   # [wrong-import-position]
import a   # [wrong-import-position]

Tbf, I don't think we should sweat other this, maybe we need to abandon the pylint disable system for this specific error message, and take the isort/ruff "I" disable into account, or drop the message entirely, or integrate ruff plus autofix and drop isort, or integrate both ruff and isort... I don't see a fix for this being super impactful in the mid term.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand your comment, and I think taking into accounts other tools setting is bit too much work for @emmanuel-ferdman 😅

@emmanuel-ferdman Do you think we can fix your issue while still keeping the behaviour as being tested in the test? I agree that a disable on a non-import makes no sense, but perhaps we can fix it with a smaller scope?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that the effort to fix this at all is not worth it as we have ruff and isort for reordering import, having working disable in pylint or coordinating multiple tools that check this message does not bring a lot of value imo. (Maybe we can even remove the check entirely for 4.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielNoord @Pierre-Sassoulas Thanks for the feedback 🙌
From issue #10589 and seeing that other pylint rules are line-based, I assumed this was a bug. But I understand that wrong-import-position might be different since it's about import ordering patterns. What should we do here? I'm happy to try and implement whatever you think is best.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for marking an issue as ready for PR when clearly it wasn't well specified @emmanuel-ferdman. Or thank you for making the fix and making us realize what actually fixing this bug imply. I think we should close because there's probably better use for your time. Right now I'm more and more convinced that we should either drop isort and wrong-import-position (import time at startup could make this a big change for users) or add ruff (using ruff and isort as optional dependencies instead of isort as mandatory dependency would open a world of possibilities once the wrong-import-position proof of concept is validated). In all case this mean this fix is not going to be used much. Need some other maintainers opinion about it and it's probably an idea for pylint 5.0 anyway (that I should open an issue for if Daniel don't think it's trash).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with removing the dependency on isort.

It fails logical to let those concerns be dealt with by a separate tool with its own configuration and/or ignores. Wondering what other maintainers think.

self.add_message(
"wrong-import-position", node=node, args=node.as_string()
)
Expand Down
9 changes: 6 additions & 3 deletions tests/functional/d/disable_wrong_import_position.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
"""Checks that disabling 'wrong-import-position' on a statement prevents it from
invalidating subsequent imports."""
"""Checks that disabling 'wrong-import-position' only affects the specific line.

A pragma on a non-import statement should not affect subsequent import statements.
This demonstrates the correct behavior after fixing the bug.
"""
# pylint: disable=unused-import

CONSTANT = True # pylint: disable=wrong-import-position

import sys
import sys # [wrong-import-position]
1 change: 1 addition & 0 deletions tests/functional/d/disable_wrong_import_position.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
wrong-import-position:10:0:10:10::"Import ""import sys"" should be placed at the top of the module":UNDEFINED
23 changes: 23 additions & 0 deletions tests/functional/w/wrong_import_position_pragma_scope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
"""Test that wrong-import-position pragma suppression is correctly scoped."""
# pylint: disable=unused-import,invalid-name

import logging
import sys

# This pragma should not affect subsequent import statements
logger = logging.getLogger() # pylint: disable=wrong-import-position
logging.basicConfig(level='DEBUG')

logger.debug('importing modules...')
import os # [wrong-import-position]
import pathlib # [wrong-import-position]
import random # [wrong-import-position]
logger.debug('done importing')

# Test that pragma on import line works correctly (this import should not be flagged)
constant_var = "test"
import json # pylint: disable=wrong-import-position

# Test that subsequent imports are not affected by the pragma above
import csv # [wrong-import-position]
import re # [wrong-import-position]
Comment on lines +19 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the correct behaviour, or at least should be considered a breaking change?

The test you're changing above seems to explicitly test for this case:
A previous disable should also count for subsequent imports.

@Pierre-Sassoulas What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, yes, we reviewed independently but I think we said the same thing..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pierre-Sassoulas You approved, but this question still stands I guess? We're now actively removing support for behaviour for which we had a test. I'm not sure why we added this behaviour in the first place, but considering we are changing behaviour isn't this a breaking change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we didn't say the same thing after all. It seems we can't have an easy implementation of both "disable on a statement" and "any disable prevent all further wrong-import-position" so there's a decision to take here. And we might have to do the hard implementation, especially since I don't think isort or ruff deal with intertwined statements in imports (?).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current PR does:

The fix changes the message enablement check from using the first non-import node's line number to using the current import node's line number, ensuring proper pragma isolation.

Perhaps we can it to:

The fix changes the message enablement check from no longer working on non-import statements for this message. A pragma will disable from the current import statement until further statements, ensuring proper pragma isolation.

5 changes: 5 additions & 0 deletions tests/functional/w/wrong_import_position_pragma_scope.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
wrong-import-position:12:0:12:9::"Import ""import os"" should be placed at the top of the module":UNDEFINED
wrong-import-position:13:0:13:14::"Import ""import pathlib"" should be placed at the top of the module":UNDEFINED
wrong-import-position:14:0:14:13::"Import ""import random"" should be placed at the top of the module":UNDEFINED
wrong-import-position:22:0:22:10::"Import ""import csv"" should be placed at the top of the module":UNDEFINED
wrong-import-position:23:0:23:9::"Import ""import re"" should be placed at the top of the module":UNDEFINED
Loading