Clearer reimported and new shadowed-import messages for aliased import

[clavedeluna]

Type of Changes

	Type
✓	🐛 Bug fix

Description

Closes #4836

Issue brought up the fact that the reimported message was confusing and hinted at something else, while also giving us more test cases to improve the check for imports using aliases.

[Pierre-Sassoulas]

LGTM, I would go further and rename reimported to something like import-name-clashor something clearer about what the problem is. "reimported" feel like what should be raised for:

from pathlib import Path
from pathlib import Path

Which is a lot less problematic than the problem we detect here.

[Pierre-Sassoulas]

Suggested change

	# pylint: disable=missing-docstring,unused-import,import-error, wildcard-import,unused-wildcard-import,redefined-builtin,no-name-in-module,ungrouped-imports,wrong-import-order,wrong-import-position,consider-using-from-import
	# pylint: disable=missing-docstring,unused-import,import-error, wildcard-import,unused-wildcard-import,redefined-builtin,no-name-in-module
	# pylint: disable=ungrouped-imports, wrong-import-order,wrong-import-position,consider-using-from-import

[coveralls]

Pull Request Test Coverage Report for Build 3452479310

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.0008%) to 95.383%

---

Totals
Change from base Build 3452308194:	0.0008%
Covered Lines:	17292
Relevant Lines:	18129

---

💛 - Coveralls

[Pierre-Sassoulas]

I realize by reading the issue that we already had this discussion. Now that I think about it again this feel like a new check, which should have a different message and probably also a different level (warning at least, while reimported is probably just a convention).

[clavedeluna]

I followed the conclusion of the issue. If maintainers would like to change scope, lmk I will close this.

[Pierre-Sassoulas]

Let's pursue the discussion in the original issue. I feel that your implementation would permit to separate the two use case relatively easily though.

[Pierre-Sassoulas]

Let's separate the two checks between reimported and shadowed-import. I also think that this is a false positive for shadowed-import as the import is not shadowed:

from pandas._libs import (
    algos as libalgos,
    hashtable as ht,
)
import pandas.core.algorithms as algos

But this should raise shadowed-import:

from pandas._libs import (
    algos,
    hashtable as ht,
)
import pandas.core.algorithms as algos  # [shadowed-import]

And this should raise reimported:

from pandas._libs import (
    algos as libalgos,
    hashtable as ht,
)
import pandas._libs.algos as algos  # [reimported]

[clavedeluna]

As a principle, I don't like changing the scope of a PR after work that could be merged is complete. This PR could be merged as is, and through an iterative process, a new issue can be opened to then separate the two checks. Without this, you're asking a contributor to do more work than they initially signed up for in an issue. So not saying scope can't change -it always will. Just saying that we should manage it by cutting it with issues.

[Pierre-Sassoulas]

Sorry about that but I don't think this is mergeable as is. This would make reimported confusing by adding a new use case that does not correspond to the current name and we'd have to make real breaking change to correct it later if we release it. (a user disable reimported somewhere and the message then change to import-shadowed and she has to disable it again) So if we merge this it's going to make the main branch un-realeasable until someone fix it and that's not something I want to have to keep in mind.

[Pierre-Sassoulas]

Looks pretty great 👍 Some minor nitpicks and let's merge !

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 4e58ebe

[cdce8p]

I don't think we should burry the new message in the bugfix section. That's a case where to changelog entries would make sense. One bugfix and one new_check.

[Pierre-Sassoulas]

#7903