-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add regression test for #6497 #6498
Add regression test for #6497 #6498
Conversation
Pull Request Test Coverage Report for Build 2296059200
π - Coveralls |
Hmm, we have other functional tests using |
Think it makes sense to do this as not finding modules can have more effect than a simple bad-open-mode false positive. |
I think it makes sense, we want to make sure that the fix works. We already got an issue with 2.13.7, and the 2.13 maintenance branch is even more different than 2.14.0-dev0 now. It could also make sense to wait for 2.14 though, it's getting closer. |
I'm tempted to wait and just do 2.14 final. May I ask what is the issue with 2.13.8 you had in mind? (Just making sure it's not the one I closed as a dupe!) |
We had an issue with the config that changed so cherry-picking went wrong https://github.com/PyCQA/pylint/milestone/64?closed=1, it's fixed now, but this kind of problem can happens again (well we'll fix it before releasing this time, but it's more work). |
requirements_test.txt
Outdated
@@ -10,3 +10,4 @@ pytest-profiling~=1.7 | |||
pytest-xdist~=2.5 | |||
# Type packages for mypy | |||
types-pkg_resources==0.1.3 | |||
numpy~=1.22.3 |
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 don't think we should add numpy to our standard test requirements, if we do that then we could have code that misbehave if numpy is not installed and we would not know. What we could do is create a CI job where we install additional libraries. It's been a recurring topic when dealing with pandas/numpy, etc. To be clear we need to be able to handle crashes in external lib and make sure that we do not regress but for false positives it's a little blurrier. For django and pytest we deferred to external plugins for false positives. Starting to do lib specific code directly in pylint is a slipery slope, we already have a lot on our plates as it is.
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 bet I could just rewrite with a fake_package/distutils
.
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.
Nah, that didn't 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.
I added a comment. Adding a new workflow is not a very appealing ROI right now. We tested this with numpy
installed in astroid
, and if it fails somehow here in pylint
again, a contributor will notice.
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 agree about not adding a new CI job right now. Would you mind adding a test for pydantic's distutil ?
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.
π
Type of Changes
Description
Closes #6497 (depends on pylint-dev/astroid#1544)
Backporting an astroid patch upgrade is not essential, but possible?