Skip to content

PR: Add Qsci to the imports#372

Merged
dalthviz merged 6 commits intospyder-ide:masterfrom
dgoeries:add-qsci
Oct 12, 2022
Merged

PR: Add Qsci to the imports#372
dalthviz merged 6 commits intospyder-ide:masterfrom
dgoeries:add-qsci

Conversation

@dgoeries
Copy link
Copy Markdown
Contributor

@dgoeries dgoeries commented Oct 9, 2022

This adds Qsci to the potential imports for qtpy.

Fixes #134

@dgoeries
Copy link
Copy Markdown
Contributor Author

dgoeries commented Oct 9, 2022

Have to include that in the CI. Some maintainer wants to take this quickly over for the workflow or wants to provide guidance?

@dalthviz dalthviz changed the title Add Qsci to the imports PR: Add Qsci to the imports Oct 10, 2022
@dalthviz dalthviz added this to the v2.3.0 milestone Oct 10, 2022
Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@dalthviz dalthviz requested a review from CAM-Gerlach October 10, 2022 18:36
@ccordoba12 ccordoba12 changed the title PR: Add Qsci to the imports PR: Add Qsci to the imports Oct 10, 2022
Handle that QScintilla bindings might not be installed. Use conda flag in tests.

Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
@dgoeries
Copy link
Copy Markdown
Contributor Author

Thanks a lot @dalthviz !

Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the initial review @dgoeries ! I left a couple more suggestions regarding the CI setup. Other than that this LGTM 👍

Work on QScintilla default version in the CI workflow

Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dgoeries for all the work here!

Copy link
Copy Markdown
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dalthviz
Copy link
Copy Markdown
Member

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 👍

@dalthviz dalthviz merged commit e7a7060 into spyder-ide:master Oct 12, 2022
@dgoeries dgoeries deleted the add-qsci branch October 12, 2022 21:16
@dgoeries
Copy link
Copy Markdown
Contributor Author

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

QScintilla Support

3 participants