PR: Prevent segfaults when importing QtAwesome out of a of QApplication #70
Conversation
|
|
||
| return self._icon_by_painter(self.painter, api_options) | ||
| return self._icon_by_painter(self.painter, api_options) |
There was a problem hiding this comment.
Maybe the else should return an empty QIcon plus some warning?
else:
warning.warn('You need to have a running QApplication to use QtAwesome!')
return QIcon()
There was a problem hiding this comment.
I think adding the warning is fine ;-)
|
@dalthviz could we create a test that does the direct import (Like what @jitseniesen pointed out). Just create a new tests folder inside the main module, ok? |
| import pytest | ||
|
|
||
| def test_segfault_import(): | ||
| subprocess.call('python -c qtawesome') |
There was a problem hiding this comment.
Please change this to
output_number = subprocess.call('python -c qtawesome')
assert output_number == 0
Remember that you always have to assert something on tests :-)
There was a problem hiding this comment.
Should we try to import every file in the submodule also?
There was a problem hiding this comment.
The most important import's for QtAwesome are on its __init__.py, so I don't see the need for that.
| @@ -0,0 +1,14 @@ | |||
| r""" | |||
| Tests for QtAwesome | |||
There was a problem hiding this comment.
dot at the end of a docstring (it is a sentence)
There was a problem hiding this comment.
We don't have to be too strict about style on test files, but sure, why not :-)
ccordoba12
changed the title
|
@ghisvail, this is kind of important in case you want to pick it up for Debian. |
|
@dalthviz, please also add the new tests folder to https://github.com/spyder-ide/qtpy/blob/master/MANIFEST.in#L5 That's necessary to add our tests to our released packages too, which was requested by @ghisvail for Debian. |
|
Then can we also update the CI code to run the test? https://github.com/spyder-ide/qtawesome/blob/master/appveyor.yml#L60 https://github.com/spyder-ide/qtawesome/blob/master/circle.yml |
|
Ok, sure. All of that have to be part of this PR. @dalthviz, sorry for putting you more work ;-) |
|
I can cherry-pick this, but have to wait for the last upload of qtawesome-0.4.3 to transition with the rest of the stack, i.e. qtpy-1.2.1 and spyder-3.1.2. Otherwise, none of them will make it for the freeze. |
|
@dalthviz, I know why our tests are failing. I'll help you to fix them :-) |
|
Ok, I think this one is ready! Thanks @dalthviz for your hard work to solve this nasty problem :-) |
Fixes spyder-ide/spyder#4014
Fixes spyder-ide/spyder#4049