Provide first component of dotted path to namespace searches #1575
Conversation
Pull Request Test Coverage Report for Build 2409772032
💛 - Coveralls |
|
The alternative I'm looking at is just catching |
|
Will await feedback on python/cpython#93334 |
jacobtylerwalls
changed the title
KeyError during namespace package searches
for more information, see https://pre-commit.ci
|
We don't want to wait for response from |
|
Seeing the label on the CPython issue we'll need a "temporary" patch for 3.7, 3.8, 3.9. |
|
Don't we want to be sure that this is a bug? Otherwise the TODO doesn't make a lot of sense? |
|
@DanielNoord yeah, I wasn't very confident of the correct behavior here but reading python/cpython#89754 suggests that explicitly giving the topmost parent package is probably correct. I just didn't want to change it in case it could miss someone's namespace package, but I realized this is only relevant if it's not on |
jacobtylerwalls
changed the title
KeyError during namespace package searches
| @@ -0,0 +1,2 @@ | |||
| This submodule should have the same name as a directory in the root of the project that | |||
There was a problem hiding this comment.
At some point we should really move these files to a tests/data directory instead of this one...
There was a problem hiding this comment.
There is definitely some cleanup to do in the tests directory, there is also the issue of legacy functional tests. https://github.com/PyCQA/pylint/blob/main/tests/test_func.py#L117 nvm it's in pylint
| # Only relevant if package not already on sys.path | ||
| # See https://github.com/python/cpython/issues/89754 for reasoning | ||
| # Otherwise can raise bare KeyError: https://github.com/python/cpython/issues/93334 | ||
| found_spec = _find_spec_from_path(working_modname, components[0]) |
There was a problem hiding this comment.
@jacobtylerwalls Just thinking about this again. We cached this function because of importlib being slow in #1536 (comment). (My very first review comment there).
What if we try to get the spec of:
a.b.c.d.e.f.g and then a.b.c.d.e.f.h? We don't benefit from any of the caching right?
There was a problem hiding this comment.
True, but I don't know if we ever confirmed that this use of importlib was slow. It's slow if importlib is actually importing the parent modules (why we did the #1409 to revert) but the whole point of this approach was to not do that, so I'm not sure it's an issue.
I hear that! We could do another beta for pylint 2.15. |
How do you feel about astroid 2.12 in pylint 2.14 ? Possible, desirable ? Should we delay this particular MR until astroid 2.13 ? Or were you thinking of using 2.12 only in pylint 2.15 ? |
I guess I'd hope to just tidy up the changelog for 2.14 and no more functional changes, since we already had a healthy beta period.
⬆️ 💯 |
|
Rats,I missed it by a few seconds. I found more KeyErrors. I'm going to revert to the other approach. |
|
Sorry about that ! |
|
no worries, it's my fault for not immediately marking as draft! |
Steps
Description
Type of Changes
Related Issue
Fixes (unreleased) crash reported here.
My current theory is that this function should only be using the top-level package name as the search path.. EDIT: no, I'm back to thinking it's better to just catch theKeyErrorand return False.