Skip to content

Configure Qt API using config file #130

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

Merged
merged 10 commits into from
Jul 28, 2016
Merged

Configure Qt API using config file #130

merged 10 commits into from
Jul 28, 2016

Conversation

nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented May 12, 2016

Fix #129

The big work here was of course to implement lazy-loading of Qt classes from qt_compat.

@The-Compiler
Copy link
Member

Oh wow, crazy stuff! 😆 👍

What do you think about setting a flag when pytest_configure was executed (or maybe pytest has already one?), and then checking that when doing the actual Qt import (to make sure no import happened before the config was read)?

As for testing, I guess you could set up an environment with multiple Qt libraries installed so pytest-qt would pick the wrong one, then run in a subprocess (to make sure nothing is imported) with the config option, and e.g. look at the version output (or simply run a test in the subprocess) to make sure the right Qt API was picked?

@nicoddemus
Copy link
Member Author

What do you think about setting a flag when pytest_configure was executed (or maybe pytest has already one?), and then checking that when doing the actual Qt import (to make sure no import happened before the config was read)?

That does not seem necessary: all imports will happen in the set_qt_api method, and that method is called in pytest_configure already. Or do you see some scenario or that might not be True?

As for testing, I guess you could set up an environment with multiple Qt libraries installed so pytest-qt would pick the wrong one, then run in a subprocess (to make sure nothing is imported) with the config option, and e.g. look at the version output (or simply run a test in the subprocess) to make sure the right Qt API was picked?

Good idea, although I it will complicate the tox setup. But you gave me an idea: setting the ini variable to the wrong api should raise an appropriate ImportError when the plugin loads. So, in a PySide environment this ini file:

[pytest]
qt_api=pyqt4

Will fail make pytest fail with an ImportError: PyQt4, thus proving the variable is working. What do you think?

qt_compat

This change will break backward compatibility for the qt_compat module, since from pytestqt.qt_compat import QApplication (for example) will not work anymore. I think this will have to warrant a 2.0 release to follow semantic versioning consistently, even thought I don't think it will break much code out there. I will probably rename it to _qt_compat to remove it from the public API as well. Thoughts?

@The-Compiler
Copy link
Member

What do you think about setting a flag when pytest_configure was executed (or maybe pytest has already one?), and then checking that when doing the actual Qt import (to make sure no import happened before the config was read)?

That does not seem necessary: all imports will happen in the set_qt_api method, and that method is called in pytest_configure already. Or do you see some scenario or that might not be True?

You're right! I thought using qt_api.QtFoo would automatically import it no matter what, but I didn't take a close enough look.

Though a test which makes sure importing before pytest_configure fails would be nice.

As for testing, I guess you could set up an environment with multiple Qt libraries installed so pytest-qt would pick the wrong one, then run in a subprocess (to make sure nothing is imported) with the config option, and e.g. look at the version output (or simply run a test in the subprocess) to make sure the right Qt API was picked?

Good idea, although I it will complicate the tox setup. But you gave me an idea: setting the ini variable to the wrong api should raise an appropriate ImportError when the plugin loads. So, in a PySide environment this ini file:

[pytest]
qt_api=pyqt4

Will fail make pytest fail with an ImportError: PyQt4, thus proving the variable is working. What do you think?

Sounds good to me.

qt_compat

This change will break backward compatibility for the qt_compat module, since from pytestqt.qt_compat import QApplication (for example) will not work anymore. I think this will have to warrant a 2.0 release to follow semantic versioning consistently, even thought I don't think it will break much code out there. I will probably rename it to _qt_compat to remove it from the public API as well. Thoughts?

Agreed. Is there anything else which would go into a 2.0 release? What about delaying 2.0 until after the pytest sprint? Then we can discuss if there's anything else we'd like to change which would need an API break (for example, I'd like to discuss making qt_wait_signal_raising default to True).

I also thought about if we should use/contribute to one of the existing wrappers instead, like pyqode.qt - but probably it's not worth the trouble, as I'd guess @ColinDuquesnoy is more interested in the needs of pyQode (for which it was written for) 😉

@nicoddemus
Copy link
Member Author

Agreed. Is there anything else which would go into a 2.0 release? What about delaying 2.0 until after the pytest sprint?

Sure, sounds good!

for example, I'd like to discuss making qt_wait_signal_raising default to True

No discussion there! 😉 In fact it was supposed to change even before that, as stated in this note. But I agree that changing it into a 2.0 is better specially if it is not far away.

I also thought about if we should use/contribute to one of the existing wrappers instead, like pyqode.qt - but probably it's not worth the trouble

I also don't think it would be worth it: pytest-qt uses a very limited set of the Qt api that just our humble qt_compat module works well, I think (although a little ugly), and I wouldn't like to add an extra dependency without a very compelling reason.

@ColinDuquesnoy
Copy link

I also thought about if we should use/contribute to one of the existing wrappers instead, like pyqode.qt - but probably it's not worth the trouble, as I'd guess @ColinDuquesnoy is more interested in the needs of pyQode (for which it was written for) 😉

I also think it wouldn't be worth it. There are already many wrappers, nearly any project that wants to support pyside, pyqt4 and pyqt5 have their own wrappers, written for their own needs.

@The-Compiler The-Compiler modified the milestone: 2.0 Jun 25, 2016
@nicoddemus nicoddemus force-pushed the qt-api-ini branch 2 times, most recently from 2c63ffa to 020b1b5 Compare June 29, 2016 23:50
@nicoddemus nicoddemus changed the title WIP: lazy loading qt api Configure Qt API using config file Jun 29, 2016


@pytest.mark.parametrize('option_api', ['pyqt4', 'pyqt4v2', 'pyqt5', 'pyside'])
def test_qt_api_ini_config(testdir, option_api):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how I ended up testing the qt_api variable in the end

@coveralls
Copy link

coveralls commented Jun 30, 2016

Coverage Status

Coverage decreased (-6.8%) to 93.179% when pulling 020b1b5 on qt-api-ini into c8edfa2 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-6.8%) to 93.179% when pulling 020b1b5 on qt-api-ini into c8edfa2 on master.

@coveralls
Copy link

coveralls commented Jun 30, 2016

Coverage Status

Coverage decreased (-4.8%) to 95.16% when pulling 1cb63ee on qt-api-ini into c8edfa2 on master.

@coveralls
Copy link

coveralls commented Jun 30, 2016

Coverage Status

Coverage decreased (-2.0%) to 98.039% when pulling 49cf8e6 on qt-api-ini into c8edfa2 on master.

@nicoddemus nicoddemus force-pushed the qt-api-ini branch 2 times, most recently from 9547d19 to a820282 Compare June 30, 2016 02:27
@nicoddemus
Copy link
Member Author

@The-Compiler rebased and I think this is ready for review. Could you take a look when you have the time please? 😁

* Now which Qt binding ``pytest-qt`` should use can be configured by the ``qt_api`` config option.
Thanks `@The-Compiler`_ for the request (`#129`_).

- ``PYTEST_QT_FORCE_PYQT`` environment variable is no longer supported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be under "breaking changes"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, bad merge, thanks!

@coveralls
Copy link

coveralls commented Jun 30, 2016

Coverage Status

Coverage decreased (-0.1%) to 99.891% when pulling 4624c0f on qt-api-ini into cef8b39 on master.

@coveralls
Copy link

coveralls commented Jun 30, 2016

Coverage Status

Coverage decreased (-0.1%) to 99.891% when pulling 7f70537 on qt-api-ini into cef8b39 on master.

@coveralls
Copy link

coveralls commented Jun 30, 2016

Coverage Status

Coverage decreased (-0.1%) to 99.89% when pulling 2205d78 on qt-api-ini into cef8b39 on master.

@The-Compiler
Copy link
Member

heh, I guess that works too 😆

@nicoddemus
Copy link
Member Author

I really don't have a clue why those lines are reported as missing... 😭

@The-Compiler
Copy link
Member

Are they covered when you do something like PYTEST_QT_API=pyqt4v2 coverage run --source=pytestqt py.test ; coverage html locally?

I'll take another look tomorrow.

@nicoddemus
Copy link
Member Author

@The-Compiler, I plan to merge this on the weekend after I figure out why coverage decreased.

Just wanted to know if you will find the time to get waitCallback (is that what we decided to call it?) into 2.0 as well? Otherwise after this PR is merged we can make the release. 😁

@The-Compiler
Copy link
Member

Probably not, I'd expect to work on it in a few days to weeks - so let's release 2.0 without it and then do a 2.1 at some point 😉

@nicoddemus
Copy link
Member Author

Roger! 👍

@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 1225cdf on qt-api-ini into cef8b39 on master.

@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling d9c3d8f on qt-api-ini into 40c3e63 on master.

@nicoddemus
Copy link
Member Author

Duh, I was looking at the wrong line.

Will merge after #141 to apply the proper changes to that branch myself.

@coveralls
Copy link

coveralls commented Jul 28, 2016

Coverage Status

Coverage increased (+0.01%) to 99.897% when pulling e160714 on qt-api-ini into bad3a3b on master.

@coveralls
Copy link

coveralls commented Jul 28, 2016

Coverage Status

Coverage increased (+0.01%) to 99.897% when pulling 3f7dd89 on qt-api-ini into dfb32bc on master.

@coveralls
Copy link

coveralls commented Jul 28, 2016

Coverage Status

Coverage increased (+0.1%) to 100.0% when pulling 667feb2 on qt-api-ini into dfb32bc on master.

@nicoddemus nicoddemus merged commit 9b1a475 into master Jul 28, 2016
@nicoddemus nicoddemus deleted the qt-api-ini branch July 28, 2016 23:14
@estan
Copy link
Contributor

estan commented Aug 12, 2016

@nicoddemus @The-Compiler: Thanks! The new option works great. I was the one who initially bugged @The-Compiler on IRC about this :)

@nicoddemus
Copy link
Member Author

👍

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 this pull request may close these issues.

5 participants