Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: Support linting in out-of-source directories#9721

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
gremat wants to merge2 commits intopylint-dev:main
base:main
Choose a base branch
Loading
fromgremat:feat/support-lintint-in-out-of-source-directories

Conversation

gremat
Copy link

@grematgremat commentedJun 10, 2024
edited
Loading

Type of Changes

Type
✨ New feature

Description

Linting files/modules that import external modules works just fine by adding the paths containing those modules toPYTHONPATH. However, there are some well-known situations where a mostly automated inclusion ofcertain 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, likepytest, 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:

Refs#9507
Refs#7357
Refs#5644

P.S.: Credits to the authors of71b6325 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 doesnot 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.

@grematgremat requested a review fromDudeNr33 as acode ownerJune 10, 2024 11:17
@grematgrematforce-pushed thefeat/support-lintint-in-out-of-source-directories branch from5646d29 to451b675CompareJune 10, 2024 12:36
@gremat
Copy link
Author

FYI: Changelog checks fails because of:actions/setup-python#886 .

@github-actionsGitHub Actions

This comment has been minimized.

@grematgrematforce-pushed thefeat/support-lintint-in-out-of-source-directories branch froma3162e1 to3f62568CompareJune 10, 2024 14:12
@codecovCodecov
Copy link

codecovbot commentedJun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.85%. Comparing base(3f1f7b8) to head(3f62568).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@##             main    #9721   +/-   ##=======================================  Coverage   95.84%   95.85%           =======================================  Files         174      174             Lines       18865    18868    +3     =======================================+ Hits        18082    18085    +3  Misses        783      783
FilesCoverage Δ
pylint/lint/__init__.py91.66% <100.00%> (ø)
pylint/lint/base_options.py100.00% <ø> (ø)
pylint/lint/parallel.py92.85% <100.00%> (ø)
pylint/lint/pylinter.py96.66% <100.00%> (+<0.01%)⬆️
pylint/lint/utils.py96.07% <100.00%> (+0.16%)⬆️

@github-actionsGitHub Actions
Copy link
Contributor

🤖 According to the primer, this change hasno effect on the checked open source code. 🤖🎉

This comment was generated for commit3f62568

@DudeNr33DudeNr33 removed their request for reviewJune 15, 2024 10:38
@DanielNoord
Copy link
Collaborator

I don't really understand why we would provide another method to manipulatePYTHONPATH 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 upPYTHONPATH 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?

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.

@gremat
Copy link
Author

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 thesrc folder (as suggested in#5644),pylint's own pylintrcwill have to do something about the import situation if you donot want to see import errors when editing files intests. Currently, this would be theinit-hook workaround as you mentioned.

I understand that we want to make this easier for users, but as far as I understand it setting upPYTHONPATH 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 ratherhacky and ugly. Compare, e.g., solutions of other parties likepytest,mypy,isort orsetuptools which all provide a nice and clean way with a specific argument/configuration option to tackle thisstandard project layout. (Nowadays, that is; seelink 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 yourpyproject.toml.

I don't really understand why we would provide another method to manipulatePYTHONPATH when users have multiple ways to do so.

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 youcould setPYTHONPATH externally, e.g.,PYTHONPATH=./src pylint. But I see two problems with that. Firstly, you will need toconvince 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 troublefinding 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.

@DanielNoord
Copy link
Collaborator

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 apip install -e . before callingpylint? In that setup, the tests directory won't be able to import frompylint unless it is installed. Changing thePYTHONPATH to act as if it can seems incorrect.

Compare, e.g., solutions of other parties

I think themypy 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 theinit-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 frommypy). 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 youcould setPYTHONPATH externally, e.g.,PYTHONPATH=./src pylint. But I see two problems with that. Firstly, you will need toconvince 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 troublefinding 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 settingPYTHONPATH 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 ofpylint? If there is then that might convince me to be more open to this change!

@akamat10
Copy link
Contributor

akamat10 commentedSep 29, 2024
edited
Loading

#9967 is alternative way of solving this problem. Instead of falling back to legacy__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.

@Pierre-SassoulasPierre-Sassoulas added Enhancement ✨Improvement to a component Waiting on authorIndicate that maintainers are waiting for a message of the author Needs decision 🔒Needs a decision before implemention or rejection labelsMar 3, 2025
@github-actionsGitHub Actions
Copy link
Contributor

This PR needs take over because because it has been open 8 weeks with no activity.

@github-actionsgithub-actionsbot added the Needs take over 🛎️Orignal implementer went away but the code could be salvaged. labelJun 16, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement ✨Improvement to a componentNeeds decision 🔒Needs a decision before implemention or rejectionNeeds take over 🛎️Orignal implementer went away but the code could be salvaged.Waiting on authorIndicate that maintainers are waiting for a message of the author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@gremat@DanielNoord@akamat10@Pierre-Sassoulas

[8]ページ先頭

©2009-2025 Movatter.jp