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.
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dependency on
pkg_resources
fromsetuptools
#1536Remove dependency on
pkg_resources
fromsetuptools
#1536Changes from 16 commits
22bca18
f53159e
0c536f1
52eb095
3175f0d
c8e2764
46a5d29
cbb47d9
4394f6d
ee77bfd
d116d52
2054a56
2697e10
c46e443
6d088ab
7aa7da4
fa60940
d0b90f4
f30e100
3dbe761
d92c48e
01b00a2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaaand with better distinguishing between builtins and namespace packages, we ... don't even need this anymore? Highlighting in a comment in case we pop the hood on this again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these tests still fail without certain code? I thought these lines were needed to make these packages namespace packages in a hacky way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if you just make
is_namespace()
unconditionally return False, these tests fail. My understanding is that the hacks were just for the reliance on the private variables ofpkg_resources
. (Part of the reasonpkg_resources
is discouraged now, I take it!)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't gotten around to testing it, but are we sure this still catches namespace packages defined in the
pkg_resources
way? I know I had made a test package when I initially worked on this and was then able to keep the same tests. See: https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#pkg-resources-style-namespace-packagesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestPackage.zip
The above zip contains a package as defined by that guideline. With the following diff:
And the following command:
pylint ../namespace_package/myotherpackage
main
returns:Whereas this PR returns:
Without the
if
:mynamespace
is thus interpreted as a different type of package. I don't know if this is actually a bug or if we are actually fixing a bug here, but I would need to do some more investigation before signing off on this. Perhaps you already know why this is different and whether this is a good thing from your own investigations?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this. I see the same output on
main
vs. the PR. Would you mind checking one more time? I also tried varying mycwd
and invokingpylint
differently.I see
PKG_DIRECTORY: 3
formynamespace
andPY_NAMESPACE: 10
formyotherpackage
on both:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is misread your question and thought you asked for my config. Well, there you have it 😄
I believe
cd
into the folder is required for this "test" to work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OH. This is the part I wasn't expecting:
python -m pip install -e .
So the test case has to do with installing the package under site-packages. I wasn't expecting that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess that is required for
importlib
to figure out thatmynamespace
is indeed a namespace package (although I don't know the specifics).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible it's the other way around. Installing it under site-packages with pip is enough to get astroid's
ImportlibFinder
to find it first rather thanExplicitNamespacePackageFinder
. And in that case, if installed withpip
, it is a regular package, not a namespace package. So we might have fixed something here if this wasn't happening before.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hm, that might actually be true. I just assumed changed behaviour was a regression.
I am not completely sure though. So I'll need to play around with this a little bit more to be sure. Should find time for this this week!