Change order of search path to fix inconsistency between pylint and astroid. #2589
Conversation
|
Also, both |
Pierre-Sassoulas
added
Enhancement β¨
|
Yeah let's make pylint rely on astroid's fonction or at least comment on the similarity if we want to reduce coupling. |
Codecov ReportAll modified and coverable lines are covered by tests β
Additional details and impacted files@@ Coverage Diff @@
## main #2589 +/- ##
==========================================
+ Coverage 93.24% 93.25% +0.01%
==========================================
Files 93 93
Lines 11049 11068 +19
==========================================
+ Hits 10303 10322 +19
Misses 746 746
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Great! Let me add a test with this PR before this is merged. |
akamat10
force-pushed
the
fix_path_search_order
branch
from
0417a67
to
8dbb594
Compare
tests/test_modutils.py
Outdated
| modutils.modpath_from_file(f"{sub_dir}/{mod_name}.py", [sub_dir]), | ||
| [mod_name], | ||
| ) | ||
| finally: |
There was a problem hiding this comment.
I think elsewhere we use a context manager for sys path modification in tests. Sorry I can't search as I'm on mobile.
There was a problem hiding this comment.
Will check. Don't remember seeing one in astroid repo. There is one in pylint repo. Perhaps you are remembering that one? If there isn't one, you want me to create one similar to that?
There was a problem hiding this comment.
Didn't find it. Will create one.
There was a problem hiding this comment.
There was a problem hiding this comment.
Sorry I did not realize it was an astroid PR π
akamat10
force-pushed
the
fix_path_search_order
branch
from
77fb51a
to
0ee04e7
Compare
for more information, see https://pre-commit.ci
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def augmented_sys_path(additional_paths: Sequence[str]) -> Iterator[None]: |
There was a problem hiding this comment.
Could we do a follow up where we move this into the test directory. By putting it here we add it to our global API, whereas it is only used in tests and we don't want to have to keep supporting it.
There was a problem hiding this comment.
Good point. I saw that it is public for pylint and followed that option. However, what you are saying makes sense. Another option is to mark it private by pretending it with _. That would then be available for other modules in astroid. Open to either one.
There was a problem hiding this comment.
Based on the experience I had with deprecating stuff with astroid and pylint 2.x I'd say move it to tests. If we ever need it we can always move it :)
There was a problem hiding this comment.
Sounds good! Will change.
Type of Changes
Description
While testing pylint-dev/pylint#9967, I noticed that the qualified package names are incorrect when linted files are outside of the source-roots specified. This PR changes the priority of the search path which then correctly picks up the right qualified module name. It makes sense to give higher priority to the additional search paths over
sys.path. This is in line with priority order of expand_modules function in pylint. Hopefully the maintainers agree that this inconsistency needs to be fixed.Refs pylint-dev/pylint#9955