-
-
Notifications
You must be signed in to change notification settings - Fork 306
Refactor Qt brain transforms to handle PyQt and PySide #2863
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
Open
emmanuel-ferdman
wants to merge
1
commit into
pylint-dev:main
Choose a base branch
from
emmanuel-ferdman:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,16 +8,13 @@ | |
|
||
from astroid import Uninferable, extract_node | ||
from astroid.bases import UnboundMethod | ||
from astroid.const import PY312_PLUS | ||
from astroid.manager import AstroidManager | ||
from astroid.nodes import FunctionDef | ||
|
||
HAS_PYQT6 = find_spec("PyQt6") | ||
|
||
|
||
@pytest.mark.skipif(HAS_PYQT6 is None, reason="These tests require the PyQt6 library.") | ||
# TODO: enable for Python 3.12 as soon as PyQt6 release is compatible | ||
@pytest.mark.skipif(PY312_PLUS, reason="This test was segfaulting with Python 3.12.") | ||
Comment on lines
-19
to
-20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π |
||
class TestBrainQt: | ||
AstroidManager.brain["extension_package_whitelist"] = {"PyQt6"} # noqa: RUF012 | ||
|
||
|
@@ -73,3 +70,17 @@ def test_slot_disconnect_no_args() -> None: | |
pytest.skip("PyQt6 C bindings may not be installed?") | ||
assert isinstance(attribute_node, FunctionDef) | ||
assert attribute_node.args.defaults | ||
|
||
@staticmethod | ||
def test_pyqt_signal_instance_connect_available() -> None: | ||
"""Test pyqtSignal() instances expose connect.""" | ||
src = """ | ||
from PyQt6.QtCore import pyqtSignal | ||
sig = pyqtSignal() | ||
sig.connect #@ | ||
""" | ||
node = extract_node(src) | ||
attribute_node = node.inferred()[0] | ||
if attribute_node is Uninferable: | ||
pytest.skip("PyQt6 C bindings may not be installed?") | ||
assert isinstance(attribute_node, FunctionDef) |
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.
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.
Should we add a matrix for pyqt 5 / 6 ?
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.
@Pierre-Sassoulas I think I can add a matrix for PyQt5/6 if you think thatβs the right approach - though they share most of the same code paths now. I could also try adding a PySide6 matrix, which might be more valuable since it goes through a different branch than PyQt, but it would require installing different libraries in the ci.
Also, currently, we have PyQt6 in the requirements file. If we decide to go with a matrix, Iβm not sure whether the right approach is to include both PyQt6 and PySide6 in the requirements or to install them dynamically during CI. Let me know what you prefer, and Iβll implement 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.
Would you be open to create a proof of concept of astroid plugin ? I.e. a repo containing the pyqt brain that we add as an optional dependency to pylint or astroid later (*) and that we extensively test in it's own repo with as much dependency as required (pyqt from 1 to 6, anything is possible because the test won't slow down the main repo). If you are then the question on the other comment is moot because this is clearly the superior way to do thing, we're just limited by available maintainer time to do it.
(*) Not sure for this part the brain need astroid so if astroid has an optional dependency to it it's going to be a circular one ?