Create additional namespaces for subdirectories with pylint configs #9395
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pierre-Sassoulas
added
Enhancement ✨
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9395 +/- ##
==========================================
- Coverage 95.82% 95.80% -0.03%
==========================================
Files 173 173
Lines 18813 18868 +55
==========================================
+ Hits 18028 18076 +48
- Misses 785 792 +7
|
This comment has been minimized.
This comment has been minimized.
0nf
force-pushed
the
per_directory_configs
branch
from
6b5f217
to
824e442
Compare
|
Thanks for the work on this. Before diving in to the nitty gritty of the code itself, I think we might want to have a short discussion about Personally I would be in favour of just only adding |
This comment has been minimized.
This comment has been minimized.
0nf
force-pushed
the
per_directory_configs
branch
from
824e442
to
98e525f
Compare
|
I noticed some bugs in previous version of my pr, added new testcases and fixed them. This one should be fine. As for |
0nf
commented
Feb 2, 2024
| MANAGER.always_load_extensions = self.config.unsafe_load_any_extension | ||
| MANAGER.max_inferable_values = self.config.limit_inference_results | ||
| MANAGER.extension_package_whitelist.update(self.config.extension_pkg_allow_list) | ||
| if self.config.extension_pkg_whitelist: | ||
| MANAGER.extension_package_whitelist.update( | ||
| self.config.extension_pkg_whitelist | ||
| ) | ||
| self.stats.reset_message_count() |
There was a problem hiding this comment.
In order to open checkers with per-file configs we need to call _astroid_module_checker per file => we need to call _get_asts before _astroid_module_checker => we need to save stats that are generated during _get_asts.
It seems that just not calling reset_message_count() has the same effect as something like self.total_stats = merge_stats(self.total_stats, self.stats) before reset_message_count()
Although I can understand both of these cases I think it's very likely that |
This comment has been minimized.
This comment has been minimized.
|
Could you advise how to handle errors from Checks / pylint (pull_request)? All errors are about spelling the words 'configs', 'subpackage' and 'cli'. |
You can add missing words here: https://github.com/pylint-dev/pylint/blob/main/.pyenchant_pylint_custom_dict.txt |
0nf
force-pushed
the
per_directory_configs
branch
from
98e525f
to
6a5b411
Compare
|
Removed use-parent-configs and made the traversal the default in separate commit. There were few changes to the pylint code itself, the most part of the PR is new tests anyways ☺ |
Sorry I don't understand what you mean by "traversal", do you mind giving an example ? |
This comment has been minimized.
This comment has been minimized.
0nf
force-pushed
the
per_directory_configs
branch
from
6a5b411
to
f1b25d3
Compare
|
@Pierre-Sassoulas As far as I understood, by traversal @DanielNoord meant searching for config files in the parent directories of linted files. Example:
|
|
Can I ask someone for a review? Maybe there is something that I could do to make a review easier? |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Yes, by "new message" I mean some text about active config right before checkers messages for given module. It should not be too hard, I'll try to push a new commit today. Another problem is that an old message "Using config file ..." can be misleading for user in case of several config files. It's printed at the time of config file parsing, but each config is parsed only once, so the message will be skipped for files that use config discovered earlier. |
|
Hey @0nf thanks for updates! Really appreciate the follow-through here. So I guess my comment earlier this week was less about the message printed to the console, and more about what it revealed to me was happening. The example I posted revealed to me that the
|
|
@jacobtylerwalls Sorry, I'm a bit confused about the case you are talking about, I'd like to ask for some details.
?
Before Review-changes 3 commit, messages about local configs just were not printed with verbose flag, configs were parsed and switched silently. I hope that verbose messages after Review-changes 3 commit give more accurate information about what is going on. It will be unexpected if you see a message from checker, showing that file from astroid/ was linted using an option specified in pylint/pylintrc instead of astroid/pylintrc. Have you spotted such message from some checker? |
|
My files look like this: ~/
pylint/
pylintrc
astroid/
pylintrcIf I cd to |
|
Let me look a little closer to ensure the differences in output I'm seeing are resulting from the configuration file in use. |
|
Yeah, I confirmed by changing pylint's pylintrc to just In other words, no matter what my |
|
Well, would it be correct to say that you are checking a module from sys.path in this case? and it was parsed and used in |
0nf
force-pushed
the
per_directory_configs
branch
from
0993f8d
to
5fd935c
Compare
|
Could you repeat your test with the last version? It will print a full path to linted files, and hence make it more clear where local configs can be discovered. |
Sorry, I should have clarified that in my example both pylint and astroid are installed with
Right, this is the problem. It's not astroid's local config. with /pylint as $pwdwith /astroid as $pwd |
|
Hello! Sorry for the delay here - I'll soon add clarifications in docs and another commit that fixes score calculation with use-local-configs. Meanwhile, I have another question: will it make sense if I split all the changes from this branch in 2 distinct PRs - the one with refactorings needed for use-local-configs, and the other with new code that depends on this refactorings? Will it be useful for review? |
|
Yes, a split would definitely help get this reviewed! |
added 9 commits
April 14, 2024 16:42
- Create additional Namespaces for subdirectories with configuration files - Open checkers per-file, so they use values from local config during opening
- plus minor changes for github spellchecker - plus fix for root directory on Windows
- fixed assert on _worker_linter.reporter - added another run of per_directory_config tests with --jobs argument - rephrasing of assertion messages to better indicate intention of checks
0nf
force-pushed
the
per_directory_configs
branch
from
b0b9139
to
e69f6f4
Compare
0nf
force-pushed
the
per_directory_configs
branch
from
e69f6f4
to
6414db8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of Changes
Description
This pr is based on changes by DanielNoord and makes possible to use different configuration files for different directories. This behavior is enabled with 2 new options, described in docs. Default behavior stays the same as before.
Also I have a few concerns related to the changes I'm proposing. Could someone direct me to the right place to consult about them, if it's not here? My questions are:
I can move it to another pr or cancel this change entirely if it was only my local problem.
Closes #618