"In the face of ambiguity refuse the temptation to guess" -- Tim Peters #29
Conversation
_easycallback is hard to use indirectly to create signals at runtime, _make_signaller is more flexible & designed specifically to creat signals at runtime. _windows used it's own signal creation mechanism, and now it uses the same one as _call_soon_threadsafe.
Using import module is cleaner than __import__ in some situations. This code is much easier to read. Also eliminates the QtModule variable.
QtCore is only referenced directly by the tests, never in code except in the initializer for the loop, but everyone else receives a copy of QtCore from the loop, and never receives it at compile time.
This is a big commit, but any smaller changes would put the project into a non-working state where everything fails and things crumble and it's all terrible. The big change is QtCore, and QApplication are no longer imported by quamash at import time. Instead, the type of the app parameter to QEventLoop determines which module is imported (PySide, PyQt4, or PyQt5). This is fairly robust and should support PySide with Qt5 when (and if) it is released. QThreadExecutor now takes in a parameter that should reference QtCore.QThread. Changes to tests and CI configuration to use a --qtimpl parameter instead of the QUAMASH_QTIMPL environment variable. a new fixture called qtcore was also created so tests (mostly those that test QThreadExecutor) could access a reference to the QtCore module. TODO update README
| try: | ||
| signal = qtimpl_qtcore.Signal(*args) | ||
| except AttributeError: | ||
| signal = qtimpl_qtcore.pyqtSignal(*args) |
There was a problem hiding this comment.
(minor) To avoid shadowing errors in an implementation, I’d suggest:
try:
signal_cls = qtimpl_qtcore.Signal
except AttributeError:
signal_cls = qtimpl_qtcore.pyqtSignal
signal = signal_cls(*args)Or just going straight away with hasattr(), which seems to be the preferred way in ducktyping terms from the python code I have seen.
There was a problem hiding this comment.
I agree with your suggested code @horazont. I don't think we should go with hasattr though, after all it's easier to ask for forgiveness than permission, which is a typical rule of thumb in Python (at least this is what I learnt many years ago).
There was a problem hiding this comment.
I went a slightly different route style-wise, (I like to reduce indentations, even if it means more lines, and I like to spell out variables in words instead of weird abbreviations), but it's been pushed. see: abb907a
|
I made a few comments in the diff. The hack of using the module attribute of the application type is quite nice, as long as it works with all (supported) Qt wrappers. I would maybe factor that out into a separate function (this is quite some magic happening where you would not expect it and it is not immediately obvious what and why this is happening; a function would allow for a matching docstring), but this is just personal taste. |
| ... f = executor.submit(lambda x: 2 + x, 2) | ||
| ... r = f.result() | ||
| ... assert r == 4 | ||
| """ | ||
|
|
||
| def __init__(self, max_workers=10, parent=None): | ||
| super().__init__(parent) | ||
| def __init__(self, qthread_class, max_workers=10): |
There was a problem hiding this comment.
Doesn't look right to me to make clients of Quamash have to specify the thread class, considering QThreadExecutor is public API.
There was a problem hiding this comment.
I agree, but I'm not sure what the alternative is.
One of the ideas floated around was some sort of quamash.set_qt(PyQt5/PyQt4/PySide) type thing.
There was a problem hiding this comment.
Yeah, I think there should be a single way of saying which Qt implementation Quamash is to use. quamash.set_qt still looks good to me, since we then don't have to infer which Qt package to use. It's logically analogous to Qt widgets' refusal to be constructed unless there's a QApplication running; the complication in our case being that we don't know the type of QApplication until someone points us to the containing package.
|
Note that the failing appveyor tests (kinda) pass. py.test is returning an exit status that isn't zero, but the console output shows all tests pass. I'm able to replicate this on my end, so it's probably something that can be fixed, but I'm not entirely sure it's a problem with qumash or a problem with py.test. |
Before sending in an app value of None was allowed (and the default), but now that's not allowed, so we should guard against it. More guards would be against duck-typing. Wow. There were TWO self.__app is not None assertions.
It's crime against nature.
This way if signal_class() raises AttributeError it won't be caught.
Simplify QThreadWorker
the dependency injection stuff. This branch is quite a bit cleaner than the depinject branch (I made a depinject4realz branch and then started over again with a depinject5realz branch)
So I'm asking for comments/review of this code to see if it seems to hackish or a suitably elegant solution befitting a pull request with a Zen of Python quote as it's title.
see #16