Skip to content
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.

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

Do not append namespace packages to sys.path #6405

Merged
merged 10 commits into from
May 2, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Apr 20, 2022

  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.
  • If you used multiple emails or multiple names when contributing, add your mails
    and preferred name in script/.contributors_aliases.json

Type of Changes

Type
🐛 Bug fix

Description

Counter to #5235.

This is not finished yet as for some reason running pylint in pre-commit does not allow this fix. However, I think we should explore if this is a possible solution to the problem. There should be a way to determine whether something is a namespace package, the python import statement can do so as well.

Closes #5226, Closes #2648

@coveralls
Copy link

coveralls commented Apr 20, 2022

Pull Request Test Coverage Report for Build 2250284307

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.004%) to 95.15%

Files with Coverage Reduction New Missed Lines %
pylint/testutils/lint_module_test.py 10 86.67%
Totals Coverage Status
Change from base Build 2249501913: 0.004%
Covered Lines: 15792
Relevant Lines: 16597

💛 - Coveralls

@DanielNoord DanielNoord marked this pull request as ready for review April 20, 2022 08:18
@Pierre-Sassoulas
Copy link
Member

I think both approach are not mutually exclusive, the option is a permanent fix to let the user tell pylint what to do. But we can still make namespace automated detection better so users don't have to use the option as often.

@DanielNoord
Copy link
Collaborator Author

I think both approach are not mutually exclusive, the option is a permanent fix to let the user tell pylint what to do. But we can still make namespace automated detection better so users don't have to use the option as often.

I think we should try to avoid adding the option. There is no inherent reason why we wouldn't be able to mimic the import mechanics of python. It's difficult (for sure...), but there is nothing that blocks us from 100% copying it eventually.
When we finally do so we are then stuck with supporting an option that is not necessarily correct and blocks some of the behaviour of pylint. For example, with this change you could (in theory I think) lint both a normal and a namespace package at the same time. With the option you wouldn't be able to do so.
Rather than turning off core behaviour of pylint I'd like to fix that behaviour and not have to support turning it off down the line.

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
@Pierre-Sassoulas Pierre-Sassoulas added the Beta-Blocker 🙅🩸 If this issue is not fixed before the beta release, our blood pressure increase too much label Apr 21, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Apr 21, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

The change look nice but this is a tricky to tests and easy to break. I'm not super confident. Probably a good thing that we release the beta first here.

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, looks great and fails on main!

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

The test is clever ! Do we release the beta right now or should we first release the current patch versions of astroid and pylint ?

@DanielNoord
Copy link
Collaborator Author

The test is clever ! Do we release the beta right now or should we first release the current patch versions of astroid and pylint ?

I think we can release the beta. I don't know of any blockers in astroid. I leave the decision for the pylint patch version to you. It doesn't really block the beta, although the "monday dependency update" might trigger people to visit our repo and see the news post about the beta. That would be an argument to do the patch first and then the beta, so the beta is the last post in Releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Beta-Blocker 🙅🩸 If this issue is not fixed before the beta release, our blood pressure increase too much Bug 🪲
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support an Option to Disable sys.path patching Invalid "Module import itself" error for namespace packages.
4 participants