-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
Drop support for PySide2 #583
Conversation
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.
Seems fine other than the tests I commented on. Personally I'd still keep it for another ~2 years (Python 3.10 EOL -> not possible to run anymore on any supported Python) given that it's not really in the way. But I'm also fine with getting rid of it, given that Qt 6.0 is 4 years old already, and 6.2 (first release with e.g. QtWebEngine available) has been around for more than 3 years. Time flies!
tests/test_wait_signal.py
Outdated
@@ -913,27 +913,6 @@ def test_empty_when_no_signal(self, qtbot, signaller): | |||
pass | |||
assert blocker.all_signals_and_args == [] | |||
|
|||
def test_empty_when_no_signal_name_available(self, qtbot, signaller): |
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'm a bit confused by the current situation here (also for the test below) - is this really only relevant for PySide2, or did we just never add PySide6 to the test?
From what this looks like:
pytest-qt/src/pytestqt/wait_signal.py
Lines 104 to 123 in 89178ed
def determine_signal_name(self, potential_signal_tuple): | |
""" | |
Attempts to determine the signal's name. If the user provided the signal name as 2nd value of the tuple, this | |
name has preference. Bad values cause a ``ValueError``. | |
Otherwise it attempts to get the signal from the ``signal`` attribute of ``signal`` (which only exists for | |
PyQt signals). | |
:returns: str name of the signal, an empty string if no signal name can be determined, or raises an error | |
in case the user provided an invalid signal name manually | |
""" | |
signal_name = self._extract_signal_from_signal_tuple(potential_signal_tuple) | |
if not signal_name: | |
try: | |
signal_name = self._extract_pyqt_signal_name(potential_signal_tuple) | |
except AttributeError: | |
# not a PyQt signal | |
# -> no signal name could be determined | |
signal_name = "" | |
return signal_name |
and from [PYSIDE-1911] Provide a way to get the name of a Signal(Instance) - Qt Bug Tracker only being partially fixed (I think we never implemented the "extract from repr" part?), I believe those tests should still be run on PySide6 actually?
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.
Ahh great catch! You are right, they should be running for PySide 6 too.
Brought the tests back and updated the condition.
c6e55d6
to
7ac3f70
Compare
PySide2 is no longer maintained, with the last release being made in 2022.
7ac3f70
to
e00b4cd
Compare
PySide2 is no longer maintained, with the last release being made in 2022.
Follow-up to #572 (comment).
@The-Compiler I like the removed cruft and CI jobs, but I understand if you would like to keep the support around.