Skip to content
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 QtPdf and QtPdfWidgets #382

Merged
merged 1 commit into from
Nov 6, 2022
Merged

Conversation

jschueller
Copy link
Contributor

Closes #381

@jschueller jschueller marked this pull request as ready for review October 27, 2022 14:11
@dalthviz dalthviz added this to the v2.3.0 milestone Oct 27, 2022
@dalthviz dalthviz changed the title Add QtPdf PR: Add QtPdf Oct 27, 2022
@dalthviz dalthviz self-requested a review October 27, 2022 16:09
@dalthviz dalthviz modified the milestones: v2.4.0, v2.3.0 Oct 31, 2022
Copy link
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 @jschueller for helping with this! Checking, I think that the QtPdf and QtPdfWidgets modules are only available in PyQt6 and PySide6 >=6.4.0. Taking that into account, maybe we should put in place a try-catch for the PyQt6 and PySide6 import calls and raise a QtBindingMissingModuleError in case of an ImportError? What do you think?

Beside that, could you rebase on top of the latest master, please? That should fix the failing test.

Other than the above this LGTM 👍

@jschueller
Copy link
Contributor Author

ok, I rebased
I think ImportError is fine, what does QtBindingMissingModuleError bring to the table ?

@dalthviz
Copy link
Member

dalthviz commented Nov 1, 2022

Thanks for the quick response! And regarding raising QtBindingMissingModuleError is just to be consistent with other imports that use the specific error classes that QtPy implements and also to show that is expected for the module import to fail in some circumstances (in this case if PyQt6 or PySide6 >=6.4)

@jschueller
Copy link
Contributor Author

thtat's not what is done for other modules, let's keep it simple

Copy link
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 again @jschueller for your help with this! Thinking a little bit more, there is no need to hold this for being merged. Indeed, keeping things simple is better, and actually there is no qtpy error implemented that encapsulates the case of a module not being available in a specific binding version (maybe in the future we could add it but for the moment the ModuleNotFoundError should be fine 👍🏼 )

@dalthviz dalthviz merged commit 03476ee into spyder-ide:master Nov 6, 2022
@dalthviz dalthviz changed the title PR: Add QtPdf PR: Add QtPdf and QtPdfWidgets Nov 6, 2022
@jschueller jschueller deleted the qtpdf branch November 6, 2022 06:44
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.

Add QtPdf, QtPdfWidgets
2 participants