-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
PR: Add minimum version check for Qt6 >=6.2.0 #294
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.
Looks good to me, thanks @CAM-Gerlach!
How about introducing PYQT5_VERSION, PYSIDE2_VERSION, PYQT6_VERSION and PYSIDE6_VERSION ? l think,
is better than
and others are the same. |
Hmm, thanks for the suggestion @kumattau . My take is that it might look a little better in a few parts of the warning check code, but I'm not sure that a bit of internal-only aesthetics is worth introducing even more new package-top-level public constants (that would have to be changed with QtPy versions that added/removed binding support) which are rather redundant to each other (since only one is going to be defined at a time) and the need to define and check them would add further code complexity to a (difficult to reliably unit test) warnings check. Additionally, on the aesthetics front, adding that one addition character would push several of the lines over the 79 char limit, making it even longer, less consistent and harder to read at a glance. @dalthviz , what do you think? |
Let me explain the background of my proposal in a little more detail. In the current API, the following simply comparison
is incorrect usage (parse('6') <= parse(PYQT_VERSION) is required before comparison ), but it is difficult for users to understand this. The related public APIs is as follows.
By introducing PYQT5_VERSION, PYSIDE2_VERSION, PYQT6_VERSION and PYSIDE6_VERSION, XXX_VERSIONs become the same behavior as PYQT5, PYSIDE2, PYSIDE2, PYSIDE6 and users can use XXX_VERSIONs without mistake. Additionaly, I think introducing QT5_VERSION and QT6_VERSION is better from the API consistency perspective. The above is my personal opinion, and of course, I respect the judgment of the spyder team. |
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 @CAM-Gerlach for working on this! I left some comments/suggestions/questions to:
- Simplify the warning message
- Add more carity to the validation on Qt6 taking into account the comments of @kumattau (thanks for taking the time to check this and raising your concerns by the way 👍)
a505afe
to
7fac941
Compare
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
7fac941
to
b794ad9
Compare
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 @CAM-Gerlach for working on this! LGTM 👍
Also, thanks @kumattau for the feedback! 👍
Per the discussion in #233 , adds a minimum version check for Qt6 and each binding to ensure they are at least version 6.2.0 or higher, since we don't officially support anything under that. Also, refactors the constants a bit accordingly.
Closes #233 (again)