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

Importing qtpy should not raise exceptions #119

Closed
cpascual opened this issue Jun 16, 2017 · 10 comments · Fixed by #121
Closed

Importing qtpy should not raise exceptions #119

cpascual opened this issue Jun 16, 2017 · 10 comments · Fixed by #121
Milestone

Comments

@cpascual
Copy link
Contributor

Hi, qtpy is breaking code that does not even use it. This was already warned by @titusjan in #65 (comment)

The following is a snippet where the problem is demonstrated. A PyQt4 program that does not use qtpy but imports it (perhaps it imports some other module that imports qtpy) gets broken. If the qtpy import is removed, the program reaches the print 3 line, but if the import is done, an exception prevents reaching that line.

import sip
sip.setapi('QString', 2)
sip.setapi('QVariant', 2)
sip.setapi('QDate', 2)
sip.setapi('QDateTime', 2)
sip.setapi('QTextStream', 2)
sip.setapi('QTime', 2)
sip.setapi('QUrl', 2)

from PyQt4 import QtGui, QtCore
app = QtGui.QApplication([])

w = QtGui.QHeaderView(QtCore.Qt.Vertical)
w.setResizeMode(w.Fixed)
print 1, w.setResizeMode

# Importing qtpy.Qwidgets changes w!!! (even if never using qtpy)
import qtpy.QtWidgets

print 2, w.setResizeMode
w.setResizeMode(w.Fixed)
print 3

IMHO, simply importing qtpy should never affect programs that do not use it (in the case of taurus-org/taurus#401 , just importing spyder.widgets.editor breaks our module ).

Would it be possible to avoid patching those methods? (or at least doing something less disruptive than raising an exception?)

Cheers!

@cpascual
Copy link
Contributor Author

Just in case it helps, I created a PR (#120) that removes the exceptions.

@ccordoba12
Copy link
Member

We didn't consider the case of having packages that only support PyQt4/5 and still wanted to use a qtpy converted package, because it kind of defeats the purpose of qtpy.

May I ask why is not possible for you to use qtpy to convert your codebase, as several other projects have done?

@cpascual
Copy link
Contributor Author

Hi @ccordoba12 ,

packages that only support PyQt4/5 and still wanted to use a qtpy converted package, because it kind of defeats the purpose of qtpy

That's funny because precisely I saw the biggest value in qtpy for precisely that situation:

For example, spyder provides widgets that may be used in another library (e.g., taurus). But that library (or the programs which use that library) are not ready to move to PyQt5... then (as I saw it) thanks to qtpy, spyder can move on and support PyQt5 while still being useful to those who are tied to PyQt4.

In fact if all the projects in the dependency chain could all move to PyQt5 at the same time, I see little point in qtpy: I would switch to PyQt5 directly (yes, I would lose PySide compatibility, but given the lack of development in PySide, that is not something I would worry about)

May I ask why is not possible for you to use qtpy to convert your codebase, as several other projects have done?

In fact converting to PyQt5 is high on our TODO list, and we consider using qtpy for the above stated reason but still the people using our library (taurus) have a lot of code which cannot be migrated soon and/or in a coordinated way. And we must support them until they are ready to migrate.

If we simply migrated taurus to qtpy (with qtpy raising exceptions as it is now), we would be pushing the problem towards our users.

@cpascual
Copy link
Contributor Author

cpascual commented Jun 19, 2017

One last thought: encouraging the users to update their code would better be done with some sort of deprecation warning rather than with exceptions...
... and even in such case, I would try to avoid patching methods that may be used by people who may not even be aware that qtpy exists. IMHO patching for adding new methods is more or less ok, but modifying existing ones (even if they still do the same) is too dangerous (you never know if someone who does not know about your module is doing her own patching in the same module) and should only be done if there are very high gains and cannot be done in some better way (and "educating" users does not qualify as such IMHO)

cpascual pushed a commit to cpascual/taurus that referenced this issue Jun 20, 2017
Force an early import of qtpy and use pyqt5 API for QHeaderView in
order to work around bug taurus-org#401

See also spyder-ide/qtpy#119

Fixes taurus-org#401
@ccordoba12 ccordoba12 added this to the v1.3.1 milestone Jul 27, 2017
@spyder-ide spyder-ide deleted a comment from mottosso Jul 27, 2017
@spyder-ide spyder-ide deleted a comment from mottosso Jul 27, 2017
@goanpeca
Copy link
Member

@mottosso advertising a library with a name that is almost the same confuses users.

cpascual pushed a commit to cpascual/qtpy that referenced this issue Jul 27, 2017
Warn instead of raising an exception if QHeaderView deprecated methods
are used. Also adapt unit tests accordingly.

Fixes spyder-ide#119
@cpascual
Copy link
Contributor Author

cpascual commented Jul 27, 2017

@goanpeca : it is not my war, but I must say that I disagree with removing @mottosso 's comment.

While I can see that the comment may be promoting a similarly-named "competing" module ( Qt.py ), IMHO @mottosso 's comment was very relevant to what was discussed here and it was certainly respectful.

As to whether the comment could confuse users, I find it a weak argument since @mottosso was explicitly pointing out differences between qtpy and Qt.py with regard to the subject of this discussion. And in any case, a reply clarifying it would have been a less-confusing response than removing the comment.

Considering that both projects are FOSS, cross-advertising and shared discussions and comparisons should be encouraged, rather than suppressed .

PS: that said, I would have appreciated a "full-disclosure" statement from @mottosso in his comment about him being an author of Qt.py

@ccordoba12
Copy link
Member

ccordoba12 commented Jul 27, 2017

@goanpeca, I don't agree with erasing @mottosso's comments either. As @cpascual said, he was just pointing out to the differences between his library and ours. There's nothing bad with that.

@mottosso, please accept my apologies for this and feel free to reinstate your comment. Else I will personally do it by copying it from my email.

@goanpeca
Copy link
Member

@ccordoba12 @cpascual you are all right, erasing the comment was not the right call.

My apologies @mottosso

@ccordoba12
Copy link
Member

The erased @mottosso's comment was:

This is the one major difference between this project and Qt.py, which is used in scenarios where multiple Qt-enabled applications are running at once in the same Python interpreter; some using Qt.py, some using an original binding such as PySide, e.g. Maya, Nuke, Houdini, 3dsMax, Mari, Blender etc.

@cpascual
Copy link
Contributor Author

Thanks @ccordoba12 , and specially @goanpeca for reconsidering, apologising and reverting the erase of @mottosso's comment. It is not always easy to do.

Let's move on!

PS: I relation to the original subject of this conversation, @mottosso wrote this (and I totally agree)

@ccordoba12 ccordoba12 changed the title just importing qtpy should not have side effects Importing qtpy should not raise exceptions Aug 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants