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

PySide6 DeprecationWarning of exec_ #286

Closed
kumattau opened this issue Nov 23, 2021 · 15 comments · Fixed by #287
Closed

PySide6 DeprecationWarning of exec_ #286

kumattau opened this issue Nov 23, 2021 · 15 comments · Fixed by #287

Comments

@kumattau
Copy link
Contributor

kumattau commented Nov 23, 2021

Hi,

exec_ function is deprecated in PySide6, so the common PyQt5/PySide2 code gets deprecated warning when QT_API=pyside6.

DeprecationWarning: 'exec_' will be removed in the future. Use 'exec' instead.
  sys.exit(app.exec_())

On the other hand, PyQt6 removes exec_ but QtPy provides exec_ for the compatibility. (#259)
As a result, DeprecationWarning of exec_ occurs only when QT_API=pyside6.
Which should this DeprecationWarning be addressed by QtPy or the user application?
Note that print_ function of PySide6 may have the same problem.

@dalthviz dalthviz added this to the v2.0.0 milestone Nov 23, 2021
@dalthviz
Copy link
Member

Thanks for the feedback @kumattau ! I think that the warning should be handled by the user application although maybe we should provide the mapping for exec_ and print_ at some point for PySide6 as we do now for PyQt6 too

Just in case, what do you think @ccordoba12 @CAM-Gerlach ?

@ccordoba12
Copy link
Member

I think we should handle the DeprecationWarning because we're the ones calling exec_.

@dalthviz
Copy link
Member

I think we should handle the DeprecationWarning because we're the ones calling exec_.

I think we are not actually calling exec_ but just mapping it (actually checking the mapping is only being done for PyQt6 currently). But just in case, @kumattau is the warning being generated when importing QtPy or when you in your code actually use exec_? Let us know

@kumattau
Copy link
Contributor Author

kumattau commented Nov 23, 2021

@dalthviz,

I found this DeprecationWarning in qtawesome/icon_broswer.py

https://github.com/spyder-ide/qtawesome/blob/9602e1f3fd8a19cddc2165bf49909d95105d0667/qtawesome/icon_browser.py#L273

icon_browser.py imports QtPy, but this warning is generated from PySide6 because QtPy does nothing about exec_ of PySide6.

@kumattau
Copy link
Contributor Author

kumattau commented Nov 23, 2021

I think if this DeprecationWarning should be handled by user application, exec_ function in the user application needs to be changed like as follows to avoid this warning and support PyQt5/PyQt6/PySide2/PySide6.

    # exec_ function is deprecated in PySide6
    # Getting exec/exec_ and executing it is separated
    # because executing exec/exec_ may propagates any exceptions.
    try:
        app_exec = app.exec
    except AttributeError:
        app_exec = app.exec_
    sys.exit(app_exec())

@dalthviz
Copy link
Member

Thanks for the clarification @kumattau ! I would say that then, with QtAwesome for example, we should do the corresponding changes there, but lets see what others say.

From the QtPy part then as, I was thinking, maybe we should a similar mapping for exec and print as is being done for PyQt6 but for PySide6

@dalthviz dalthviz removed this from the v2.0.0 milestone Nov 23, 2021
@CAM-Gerlach
Copy link
Member

I agree with @dalthviz ; "handling" the warning in QtPy would mean either dropping the shim functionality (which we deliberately added knowing this), or silencing the warning, meaning downstream code (like QtAwesome) won't know it has to update and will just suddenly break someday, so either doesn't really make sense.

So, we can either simply pass the warning as we do now, or, if it doesn't raise backward compat concerns or have side effect on other users of the bindings (as in #26 ), map exec_ and print_ to their non-deprecated alternatives, with any necessary shim in between.

@kumattau
Copy link
Contributor Author

Thank @dalthviz @CAM-Gerlach @ccordoba12 for comment and explanation.
I've acknowledged.
I sended the QtAwesome PR (spyder-ide/qtawesome#186) based on the discussion in this issue.

I think this issue can be closed.

@CAM-Gerlach
Copy link
Member

I think this issue can be closed.

As @dalthviz mentioned, we could consider aliasing the new names to the old ones, so user code doesn't need to implement try-excepts depending on the binding and version used; the goal of QtPy after all is a compatibility shim to abstract those differences so they don't need to be handled in every downstream application, and so ports to different bindings and upgrades to new versions is as seamless as possible (see #62 ).

@dalthviz
Copy link
Member

Thank @dalthviz @CAM-Gerlach @ccordoba12 for comment and explanation.
I've acknowledged.
I sended the QtAwesome PR (spyder-ide/qtawesome#186) based on the discussion in this issue.

Thanks @kumattau !!

we could consider aliasing the new names to the old ones, so user code doesn't need to implement try-excepts depending on the binding and version used

Would you like to help us with that @kumattau ? Let us know!

@kumattau
Copy link
Contributor Author

kumattau commented Nov 25, 2021

Thank @CAM-Gerlach @dalthviz for more explanations.

I'm sorry to misunderstand.

we could consider aliasing the new names to the old ones, so user code doesn't need to implement try-excepts depending on the binding and version used

My current understanding is that, to be specific, user applications like as qtabrowser does not need to handle DeprecationWarning of exec_/print_ functions of PySide6 because the future QtPy release will provide exec_/print_ functions of PySide6 which does not report DeprecationWarning.

Is my understanding correct ?

I think the exec_/print_ functions is desirable to be provided in QtPy 2.0.0 release, which is going to support PySide6.

@dalthviz dalthviz added this to the v2.0.0 milestone Nov 25, 2021
@dalthviz
Copy link
Member

Yep that will be the idea 👍 The thing is that right now we are only doing the mapping on PyQt6. So we still need to add the mapping (some lines like for example QApplication.exec_ = QApplication.exec) for PySide6, and I though that maybe that is something that you can help us doing @kumattau

Sorry for not being so clear but let us know if you want to help with that or if you have any other question!

@kumattau
Copy link
Contributor Author

kumattau commented Nov 26, 2021

Note that print_ function of PySide6 may have the same problem.

I mistook about the print_ function of PySide6.
Unlike exec_, PySide6/PySide2 does not have print function. They have print_ function only.

The summary is as follows.

Qt Bindings PyQt5 PyQt6 PySide2 PySide6
exec_ v v deprecated
exec v v v
print_ v v v
print v v

I think QtPy don't have to do anything about print and print_ of PySide6 because print_ is available on PyQt5/PyQt6/PySide2/PySide6. (when QT_API=pyqt6, QtPy already has mapped print to print_)

@kumattau
Copy link
Contributor Author

I sent PR: #287 to fix this issue.

@CAM-Gerlach
Copy link
Member

Thanks for your detailed research, @kumattau !

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

Successfully merging a pull request may close this issue.

4 participants