Make FunctionDef.implicit_parameters return 1 for methods#1531
Make FunctionDef.implicit_parameters return 1 for methods#1531jacobtylerwalls merged 6 commits intopylint-dev:mainfrom
FunctionDef.implicit_parameters return 1 for methods#1531Conversation
Relatedly, make `FunctionDef.is_bound` return True for instance methods.
|
@The-Compiler does this resolve the false positive for you? |
FunctionDef.implicit_parameters return 1 for instance methodsFunctionDef.implicit_parameters return 1 for methods
| :rtype: bool | ||
| """ | ||
| return self.type == "classmethod" | ||
| return self.type in {"method", "classmethod"} |
There was a problem hiding this comment.
This method was untested according to coveralls.
Pull Request Test Coverage Report for Build 2250456664
💛 - Coveralls |
I can't say I checked every one, but I checked multiple and this didn't seem to fix any. |
| if attribute_node is Uninferable: | ||
| pytest.skip("PyQt6 C bindings may not be installed?") |
There was a problem hiding this comment.
Don't we want to force the c binding to work correctly in at least one of the CI pipeline ? Otherwise this test can work fine online and fail for someone with QT locally. I think we could create a pytest mark for "qt" and a CI job with a proper installation.
There was a problem hiding this comment.
Yes, it works in the "release tests" workflow.
There was a problem hiding this comment.
Hmm, it means we're going to catch issues at release time right ? When/how are we launching this workflow ?
There was a problem hiding this comment.
Just manually with a reminder in the release.md.
Installing qt takes 39s. Unless there's a way to cache it?
There was a problem hiding this comment.
For 40s we might as well add it for each commit (one interpreter only ?). There's a lot of available runner for astroid and they run in a reasonable time (compared to pylint anyway).
There was a problem hiding this comment.
Actually, it takes 8s, duh, because most of that linked run was setting up the env!
There was a problem hiding this comment.
I'm confused - why does "Install Qt" actually only do sudo apt-get install build-essential libgl1-mesa-dev? You are installing PyQt from binary wheels, so that 40s step can probably just be dropped entirely.
There was a problem hiding this comment.
We discovered that the PyQt6 wheel doesn't necessarily install the C bindings on ubuntu-latest, see failing run and comment.
Realized that one job was skipped if QT6 was not properly installed.
Almost! It still complains on two lines compared to 2.13.7: and The former sounds like pylint/astroid thinks a slot is needed for (should be The latter gives me |
|
Thanks for testing. Feel free to open new issues or PRs. Apparently |
* qt brain: Make slot argument optional for disconnect() Discussed here: #1531 (comment) PyQt supports calling .disconnect() without any arguments in order to disconnect all slots: https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect Strictly speaking, slot=None is a wrong call, as actually passing None does not work: python-qt-tools/PyQt5-stubs#103 However, pylint/astroid does not support overloads needed to properly express this: pylint-dev/pylint#5264 So, while this is "wrong", it's less wrong than before - without this change, pylint expects a mandatory argument, thus raising a false-positive here: from PyQt5.QtCore import QTimer t = QTimer() t.timeout.connect(lambda: None) t.timeout.disconnect() despite running fine, pylint complains: test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter) while with this change, things work fine.
* qt brain: Make slot argument optional for disconnect() Discussed here: #1531 (comment) PyQt supports calling .disconnect() without any arguments in order to disconnect all slots: https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect Strictly speaking, slot=None is a wrong call, as actually passing None does not work: python-qt-tools/PyQt5-stubs#103 However, pylint/astroid does not support overloads needed to properly express this: pylint-dev/pylint#5264 So, while this is "wrong", it's less wrong than before - without this change, pylint expects a mandatory argument, thus raising a false-positive here: from PyQt5.QtCore import QTimer t = QTimer() t.timeout.connect(lambda: None) t.timeout.disconnect() despite running fine, pylint complains: test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter) while with this change, things work fine.
* qt brain: Make slot argument optional for disconnect() Discussed here: #1531 (comment) PyQt supports calling .disconnect() without any arguments in order to disconnect all slots: https://www.riverbankcomputing.com/static/Docs/PyQt6/signals_slots.html#disconnect Strictly speaking, slot=None is a wrong call, as actually passing None does not work: python-qt-tools/PyQt5-stubs#103 However, pylint/astroid does not support overloads needed to properly express this: pylint-dev/pylint#5264 So, while this is "wrong", it's less wrong than before - without this change, pylint expects a mandatory argument, thus raising a false-positive here: from PyQt5.QtCore import QTimer t = QTimer() t.timeout.connect(lambda: None) t.timeout.disconnect() despite running fine, pylint complains: test.py:4:0: E1120: No value for argument 'slot' in method call (no-value-for-parameter) while with this change, things work fine.
Description
When a node is inferred as
FunctionDefinstead ofBoundMethod,implicit_parameters()always returns 0, which can cause problems inpylintwithno-value-for-parameterand such. We can at least fall back to the knowledge that it is bound and return 1.Type of Changes
Related Issue
Closes pylint-dev/pylint#6464 (because we will not be writing Qt tests there)
no-value-for-parameter