-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add an option to disable sys.path patching. #5235
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html | ||
| # For details: https://github.com/PyCQA/pylint/blob/main/LICENSE | ||
| # Copyright (c) https://github.com/PyCQA/pylint/blob/main/CONTRIBUTORS.txt | ||
|
|
||
| """This file is intended to only be used for testing namespace package importing. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should add file without purpose in pylint itself. pylint is not a namespace package so maybe we can create a fixture or it's not even required to have one ? Could we use a namespace that does not exists ? Or is this supposed to be in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought testutils was for files used in tests/? Are you fine with test file importing another test file? I don't think that's currently done by any of the tests and a future version of pytest will not allow that by default either. https://docs.pytest.org/en/6.2.x/pythonpath.html#import-modes So moving it to tests/functional/n/namespace_package will force you to disable an upcoming pytest change. There's a second reason why it's better to avoid having the file in tests/. sys path disable mode has a requirement either files you import are installed or you run it from the right directory. tests folder is not included in pytest install. As a side effect if I move the file to tests this new test will only work as expected if run from the repository root directory. If a user tries to run pytest from a different directory for this test it won't work. Because of those 2 reasons having the _fake file with tests/ will impose restrictions on pytest usage and I'd prefer to avoid. I can move it there, but the tests will become sensitive to where you run them and unsure that's a good trade off.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Other option is drop this test entirely and make sys path logic is tested in future primer test. The main issue for making this not directory sensitive is I need module I'm importing to be installed package in repo. The only choice for that is under pylint/ somewhere. I can make a folder _internal/ and actual location doesn't matter as long as it's in pylint.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I forgot to reply. We have some test with import in
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would putting it in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it would have same issue as anywhere outside pylint. The root issue is this line, https://github.com/PyCQA/pylint/blob/main/setup.cfg#L57 When pylint is installed as a package, it installs pylint directory. It does not install tests directory. Namespace package solution here works under one of 2 conditions, the imported modules are installed or you need to use right directory to run them from. The second condition means that for development tests will break depending on cd and will complicate test scripts. The first condition means I can't test this well if pylint separates test folder from the install. Some codebases have a test structure of keeping tests in same folder as src and tests are included with a package install. https://docs.pytest.org/en/6.2.x/goodpractices.html#tests-as-part-of-application-code pytest discusses different test folder structures here. The structure of mixing tests with application code would work. It'd need a pr to refactor the test structure. You could in theory mix both test structures but it'd be fairly confusing if some tests lived in pylint/ and other tests lived in tests/. Having every user be forced to install all tests as part of pip install is a downside to this option. For stable project to test with I'm unsure of repository that uses implicit namespace packages and would fail without this. Most repositories don't use namespace packages. For the ones that do I expect if they use pylint they have workarounds. My internal repo that needs this uses a workaround of never calling pylint on the whole repo and also has multiple setup.py in it which primer would struggle with. The primer I think is designed for repositories with single setup.py/cfg. Monorepo that has multiple separate packages (so separate setup.cfg/py) installed to same namespace I don't think mypy primer supports and that's situation most likely to need this. I can make a toy repository for this that primer would work with.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this work for primer ? https://github.com/madhavhugar/example-python-namespace-package
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. Reading the project, is true in readme, but the repository does not follow that itself. I don't see any namespace module in that repository. Instead it looks like a repository that has 3 packages all in same repository and controlled by same setup.py, but no namespace that shares them. I can make a simple example for the primer tomorrow.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you much appreciated ! |
||
|
|
||
| Importing a file has a global side effect of adding a module to sys.modules. | ||
| As namespace packages are sensitive to sys.path, to test them we check | ||
| an intentional import error and a successful import. To avoid the imports | ||
| messing up the test environment, we need to make the import error use | ||
| a file that is never imported by any other test. This serves as that file. | ||
| """ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| # pylint: disable=missing-docstring,unused-import | ||
| from pylint.testutils import _fake # [no-name-in-module] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| no-name-in-module:2:0::No name '_fake' in module 'pylint.testutils':HIGH |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| # pylint: disable=missing-docstring,unused-import | ||
| from pylint import lint |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| [master] | ||
| disable-path-patching=yes |
Uh oh!
There was an error while loading. Please reload this page.