Prevent first-party imports from being resolved to site-packages
#1756
Conversation
jacobtylerwalls
added
the
pylint-tested
Pull Request Test Coverage Report for Build 2943907425
💛 - Coveralls |
|
@DanielNoord could you check this please ? I'm thinking of releasing astroid 2.14.5 once it's merged. |
|
EDIT: fixed in 22ce69e |
There was a problem hiding this comment.
Change itself looks good, just some stylistic and trivial changes! Thanks @jacobtylerwalls!
| @@ -15,6 +15,13 @@ | |||
|
|
|||
| @lru_cache(maxsize=4096) | |||
| def is_namespace(modname: str) -> bool: | |||
| from astroid.modutils import ( # pylint: disable=import-outside-toplevel | |||
| EXT_LIB_DIRS, | |||
There was a problem hiding this comment.
Can we move these to const to remove any import cycles? We could also move the union there.
There was a problem hiding this comment.
I considered that, but there's too much logic in modutils that would have to be moved. My preference would be to move is_namespace into modutils, but I'd prefer to do that in a followup commit that wouldn't be backported. What do you think?
There was a problem hiding this comment.
I think it would be good to back port this. We never know how long it takes until we release a minor version and we might want to make more fixes here.
I'd be happy to make a back port PR 😄
There was a problem hiding this comment.
I opened #1759 we just need to merge this and cherry pick it before releasing 2.12.5.
There was a problem hiding this comment.
I think it would be good to back port this
Ah, we might be miscommunicating! I'm saying yes, we should definitely backport the crash fix, but also that I wouldn't want to block that on refactoring the cyclic import out. There's a lot of logic in modutils that would have to be moved, and it's all public, so I wouldn't want to break backwards compatibility on that in a patch release.
So I was hoping to merge and backport this in its current form, and leave the refactor for another day (and not backport the refactor, as usual). How does that sound?
There was a problem hiding this comment.
Let's merge this as is, but let's also back port the refactor. Like I said: with astroid the time in between minor versions can be quite long and we might need to fix something and release a patch version after the refactor. To avoid future merge issues I would also merge any refactor to this particular piece of code.
There was a problem hiding this comment.
I agree with @DanielNoord about cherry-picking the refactors. Case in point the 2.12 maintenance branch has been live for 2 months now. Cherry-picking things after a refactor without the refactor is a lot harder than just cherry-picking the refactor too. (We do need to make sure our refactor are just refactor, but we never had any problem with this so far).
|
@jacobtylerwalls Success! Rather than fighting with local reproduction, I fired up a test on the original place of failure, that is, GitHub Actions. I force-installed this PR the following way. The pylint run step is now clean. Many thanks for the prompt address of this issue! I really appreciate it. |
…1756) Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
…1756) Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Description
Regression in 5067f08.
Type of Changes
Related Issue
Closes pylint-dev/pylint#7365