-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Support linting in out-of-source directories #9721
base: main
Are you sure you want to change the base?
feat: Support linting in out-of-source directories #9721
Conversation
5646d29
to
451b675
Compare
FYI: Changelog checks fails because of: actions/setup-python#886 . |
This comment has been minimized.
This comment has been minimized.
a3162e1
to
3f62568
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9721 +/- ##
=======================================
Coverage 95.84% 95.85%
=======================================
Files 174 174
Lines 18865 18868 +3
=======================================
+ Hits 18082 18085 +3
Misses 783 783
|
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉 This comment was generated for commit 3f62568 |
I don't really understand why we would provide another method to manipulate I understand that we want to make this easier for users, but as far as I understand it setting up By the way, some of the PRs you link are not really related to this. For example, #5644 is just an issue with the project structure and not really related to any of the issues you mention in this PR. |
Actually, #5644 provides a very prominent - because it's pylint itself! - example for the benefit of my suggested changes. As far as I can see, and please correct me, if I'm wrong, as soon as pylint's code is moved to the
Yes,
For pylint, I am only aware of |
I'm not sure if this is correct. Wouldn't this be solved by doing a
I think the
I understand your point, and can see how it is far from optimal. On the other hand, the same argument as I provided above applies here. We can tap into and refer to existing documentation for setting I want to stress that I really do understand where you are coming from and see this as a nice feature for most users. However, from the perspective of a maintainers it seems to invite more edge cases and bugs to our tool without providing any features that the currently available features don't already provide. Perhaps I missed this so let me ask explicitly: is there anything that this solution allows you to do that you can't using the latest version of |
#9967 is alternative way of solving this problem. Instead of falling back to legacy |
Type of Changes
Description
Linting files/modules that import external modules works just fine by adding the paths containing those modules to
PYTHONPATH
. However, there are some well-known situations where a mostly automated inclusion of certain directories is beneficial. E.g., in a python project setup where source files are located insrc/
, you want to lint the tests located intests/
without import errors for the main module.Similar to other python projects, like pytest, this PR introduces the configuration option
main.pythonpath
where you can specify absolute and, especially, relative paths that will always be included tosys.path
when linting.Notes on related issues:
sys/PYTHONPATH
in our doc and on a popular stackoverflow answer #9507, i.e., use aninit-hook
.sys.path
on a per-file basis #7357 which, by now, is stale because it was introduced before the support for implicit namespace packages 71b6325 was merged.init-hook
is most likely not an option.sys.path
manipulations, e.g., Linting more than one dir or package, can hide errors or cause wrong errors #2966, Support an Option to Disable sys.path patching #5226, to name a few. Of course, caution is advised in general for those manipulations as they alter the global state. However, I don't see immediate problems with the implementations. Please speak up, if you do.Refs #9507
Refs #7357
Refs #5644
P.S.: Credits to the authors of 71b6325 which made this implementation straight forward.
EDIT:
In a first version, I've added
--pythonpath
in Pyreverse, too (though corrupted because unfortunately I missed that Pyreverse does not share config with the linter). But I gave it another thought and I don't see the point of integratingpythonpath
there, too. So I will leave it out until someone argues in favor for it.