Skip to content

PR: Add QtPdf and QtPdfWidgets#382

Merged
dalthviz merged 1 commit intospyder-ide:masterfrom
jschueller:qtpdf
Nov 6, 2022
Merged

PR: Add QtPdf and QtPdfWidgets#382
dalthviz merged 1 commit intospyder-ide:masterfrom
jschueller:qtpdf

Conversation

@jschueller
Copy link
Copy Markdown
Contributor

@jschueller jschueller commented Oct 27, 2022

Closes #381

@jschueller jschueller force-pushed the qtpdf branch 3 times, most recently from 00cfb36 to 7eb9194 Compare October 27, 2022 13:25
@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
dalthviz
dalthviz reviewed Nov 1, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment •
edited
Loading

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
Copy Markdown
Contributor Author

jschueller commented Nov 1, 2022

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

@dalthviz
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

jschueller commented Nov 5, 2022

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

dalthviz
dalthviz approved these changes Nov 6, 2022
View reviewed changes
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 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 mentioned this pull request Nov 6, 2022
Merged
@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

Reviewers

@dalthviz dalthviz dalthviz approved these changes

Assignees

@jschueller jschueller

Labels

type:Enhancement

Projects

None yet

Milestone

v2.3.0

Development

Successfully merging this pull request may close these issues.

Add QtPdf, QtPdfWidgets

2 participants

@jschueller @dalthviz