PR: Add Qsci to the imports#372
Conversation
|
Have to include that in the CI. Some maintainer wants to take this quickly over for the workflow or wants to provide guidance? |
There was a problem hiding this comment.
Thank you @dgoeries for the help with this! I left some suggestion to better handle the case when the extra packages required to import QScintilla related things are not available, and also some suggestions for the test skipif condition.
As a side note, checking the changes done here to add QScintilla to the test suite, maybe we should make the pip jobs install all the extra packages like QScintilla/PyQt6-QScintilla (packages besides PyQtWebEngine like PyQtCharts, PyQt3D and others). What do you think @CAM-Gerlach ?
Handle that QScintilla bindings might not be installed. Use conda flag in tests. Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
|
Thanks a lot @dalthviz ! |
Work on QScintilla default version in the CI workflow Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
CAM-Gerlach
left a comment
There was a problem hiding this comment.
LGTM overall, thanks @dgoeries and @dalthviz .
It is a little odd to me to install and test Qscintillla, of all add-ons (being newly added and only on one binding), but not the others. Ideally we'd have a matrix with and without optional addons, but realistically it probably makes the most sense to just test with the various add-on packages, rather than without (especially if we're just skipping the tests instead of checking the correct exception is raised, or at least strict-xfailing them), considering there are more valuable things we would want to spend our matrix on, e.g. more max and min versions.
It might also be a good idea to have an opt-in or opt-out warning of modules that are not available on all supported Qt or binding versions, for users who want to ensure full cross-version/binding compatibility (ala the purpose of QtPy). However, I don't think that's in scope here.
|
Thanks for the feedback @CAM-Gerlach ! I will proceed merging this one and in a new PR adding to the test suite setup the installation of other packages 👍 |
|
Thanks a lot! |
This adds Qsci to the potential imports for qtpy.
Fixes #134