Open gremat opened 3 months ago
FYI: Changelog checks fails because of: https://github.com/actions/setup-python/issues/886 .
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉
This comment was generated for commit a3162e17526bc4d50e33e7ac07ee6785f935efef
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 95.85%. Comparing base (
3f1f7b8
) to head (3f62568
). Report is 2 commits behind head on main.
🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉
This comment was generated for commit 3f625682839e526cdf4740066a1f38f52730bf1a
I don't really understand why we would provide another method to manipulate PYTHONPATH
when users have multiple ways to do so.
I understand that we want to make this easier for users, but as far as I understand it setting up PYTHONPATH
should be done before you call into your Python executable. The init-hook
is the earliest thing that gets executed so that is probably the best place to get this behaviour? What was your reason for not documenting the current way to do this but add another option?
By the way, some of the PRs you link are not really related to this. For example, https://github.com/pylint-dev/pylint/issues/5644 is just an issue with the project structure and not really related to any of the issues you mention in this PR.
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 src
folder (as suggested in #5644), pylint's own pylintrc will have to do something about the import situation if you do not want to see import errors when editing files in tests
. Currently, this would be the init-hook
workaround as you mentioned.
I understand that we want to make this easier for users, but as far as I understand it setting up
PYTHONPATH
should be done before you call into your Python executable. Theinit-hook
is the earliest thing that gets executed so that is probably the best place to get this behaviour? What was your reason for not documenting the current way to do this but add another option?
Yes, init-hook
might be one of the first places to hook up into. However, I consider this workaround to be rather hacky and ugly. Compare, e.g., solutions of other parties like pytest, mypy, isort or setuptools which all provide a nice and clean way with a specific argument/configuration option to tackle this standard project layout. (Nowadays, that is; see link from #5644 for older alternatives.)
That said, my reason to add another option is mainly to provide a nice, clean and similarly-looking option in pylint, too. It will get easier for (new) users, in fact, easy enough that you almost don't need to consult the documentation/help output and could easily guesstimate the required option in your pyproject.toml
.
I don't really understand why we would provide another method to manipulate
PYTHONPATH
when users have multiple ways to do so.
For pylint, I am only aware of --source-roots
to manipulate PYTHONPATH
but it serves a different purpose because it is related to implicit namespace packages and does not help with the init-hook
vs. suggested option.
You are of course right that you could set PYTHONPATH
externally, e.g., PYTHONPATH=./src pylint
. But I see two problems with that. Firstly, you will need to convince your editor/IDE of that which is again error-prone, not out-of-the-box and you have to do that for every editor you use (moreover, might be even required on a per-project basis). Secondly, you might very well get into trouble finding and specifying the right location which depends on your current working directory, the file location, the project root and from where exactly it will be called.
Thus I don't consider the external option to be a viable approach.
Actually, #5644 provides a very prominent - because it's pylint itself! - example for the benefit of my suggested changes.
I'm not sure if this is correct. Wouldn't this be solved by doing a pip install -e .
before calling pylint
? In that setup, the tests directory won't be able to import from pylint
unless it is installed. Changing the PYTHONPATH
to act as if it can seems incorrect.
Compare, e.g., solutions of other parties
I think the mypy
example is exactly what frightens me from adding this. The amount of configuration and gotcha's on different platforms seems like a headache. For example, why use globs and not regular expressions like we do for other configuration options.
What I personally like about the init-hook
is that we don't need to provide any "support". We can point users to common patterns/commands that might satisfy their needs, but there is no reason to look into our code to see whether we automatically expand home directories (to take the example from mypy
). It's just Python code.
For pylint, I am only aware of
--source-roots
to manipulatePYTHONPATH
but it serves a different purpose because it is related to implicit namespace packages and does not help with theinit-hook
vs. suggested option. You are of course right that you could setPYTHONPATH
externally, e.g.,PYTHONPATH=./src pylint
. But I see two problems with that. Firstly, you will need to convince your editor/IDE of that which is again error-prone, not out-of-the-box and you have to do that for every editor you use (moreover, might be even required on a per-project basis). Secondly, you might very well get into trouble finding and specifying the right location which depends on your current working directory, the file location, the project root and from where exactly it will be called. Thus I don't consider the external option to be a viable approach.
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 PYTHONPATH
for other tools and IDEs. Making our own custom solution increases the scope for bugs and edge cases we didn't consider.
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 pylint
? If there is then that might convince me to be more open to this change!
__init__.py
based walking up the directories when source-roots are specified, it changes the fallback behavior for --source-roots
to return the --source-roots
list if the linting files don't overlap with any of the --source-roots
. This will simplify the behavior of --source-roots
and opens it up to more usecases beyond just implicit namespace packages.
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 optionmain.pythonpath
where you can specify absolute and, especially, relative paths that will always be included tosys.path
when linting.Notes on related issues:
init-hook
.init-hook
is most likely not an option.sys.path
manipulations, e.g., #2966, #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 71b6325c8499c8a66e90d90e88c75d7c7ab13b23 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.