Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

New behavior when start import QtPy - setting Qt binding#156

Open
dpizetta wants to merge 35 commits intospyder-ide:masterfrom
dpizetta:master
Open

New behavior when start import QtPy - setting Qt binding#156
dpizetta wants to merge 35 commits intospyder-ide:masterfrom
dpizetta:master

Conversation

Copy link
Contributor

dpizetta commented May 26, 2018

This PR restructure the mainly the init file, implementing new functions to help QtPy to get info about Qt API's.

  • QtPy will always try 4 times (4 API's) if None is found.
  • Add and improve warnings if changes and fails occurs.
  • Help functions may be part of scripts to help developers in the future, ex.:
qtpy --available_api >> PySide, PyQt5
qtpy --api_info=PyQt5 >> APIVersion, QTVersion ...

More details about the new behavior are described in init documentation.

It passed tests locally, but some variables were changed in init file, that someone is using somewhere in other projects.

PYSIDE_VERSION > API_VERSION

Feel free to criticise and comment the code.

Copy link
Contributor Author

dpizetta commented May 28, 2018

About CI errors, I can change the use of importlib.utils to try...import to avoid problems with py27. Waiting for your reviews :) Tks

goanpeca requested a review from ccordoba12 May 28, 2018 14:53
Copy link
Contributor

goanpeca commented May 28, 2018

PYSIDE_VERSION > API_VERSION

Those changes might be problematic, we would need to preserve names, and maybe add deprecation notices if we want to change this for 1.5, or even 2.x ?

@ccordoba12 ?

ccordoba12 reviewed May 29, 2018
qtpy/__init__.py Outdated

>>> import os
>>> os.environ['QT_API'] = 'pyside2'
>>> os.environ['QT_API'] = 'PySide2'
Copy link
Member

ccordoba12 May 29, 2018

Choose a reason for hiding this comment

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

Sorry but I don't have much time right now to do a thorough review of this. For now, please revert this change you made here about using capitalization to set QT_API.

There's a well established convention among Python projects that deal with different Qt bindings to use QT_API as we are using it here, so please revert that.

Copy link
Contributor Author

dpizetta May 29, 2018

Choose a reason for hiding this comment

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

Ok, sorry for that, actually it was a find/replace that goes wrong. I will fix.

Copy link
Member

ccordoba12 commented May 29, 2018

@dpizetta, I left an initial comment and also please fix the failures in our CIs (the best way you consider it) before we can start to review your work.

goanpeca reviewed May 29, 2018
Copy link
Contributor Author

dpizetta commented May 30, 2018 *
edited
Loading

The fail in CI for PySide2 maybe is caused by the low speed to download the wheels. I have this problem here too. The latest version is sadly very low to download. I have used latest-1 :) Maybe enable cache can help if not enabled. #159

Copy link
Contributor Author

dpizetta commented Jun 11, 2018 *
edited
Loading

The problems with pyside2 servers seem to be solved... https://bugreports.qt.io/browse/PYSIDE-684 :)
I would like to rebuild in CircleCI, but I do not have permissions to do so. Can you try?

ccordoba12 added this to the v2.0 milestone Jun 11, 2018
Copy link
Member

ccordoba12 commented Jun 11, 2018

@dpizetta, please rebase this one in top of master to fix the error in CircleCI.

dpizetta force-pushed the master branch from 977c0e4 to 341ecd3 Compare June 20, 2018 13:26
ccordoba12 mentioned this pull request May 5, 2019
Copy link
Contributor

goanpeca commented May 5, 2019

I forgot about this. Will review it :-)

goanpeca self-assigned this May 5, 2019
Copy link
Contributor Author

dpizetta commented Jun 6, 2019

Hi guys, I would like to ask both of you @goanpeca and @ccordoba12 to wait for me to update this PR. I've also more ideas that can help this PR. Tks

Copy link
Contributor

goanpeca commented Jun 6, 2019

Ok @dpizetta thanks for working on this and sorry for the late review! Please let us know

Copy link
Contributor

goanpeca commented Jun 6, 2019 *
edited
Loading

One genera comment is that if you want to use the QT_API to import something, I think is better to let QT_API be case independent (pyside2, PySide2 etc.. should do the same thing), and have a map somewhere to use for your needs

QT_API = 'PySide2'
MAP = {
'pyside2': 'PySide2',
...
}
qt_api_module = MAP.get(QT_API.lower())

Thoughts @ccordoba12 ?

Copy link
Member

ccordoba12 commented Jun 6, 2019

That's the current model, i.e. we accept bindings in upper or lower case names (see PR #45). @dpizetta, if that's changed by your PR, please revert it.

Copy link
Contributor Author

dpizetta commented Aug 7, 2019

Hi guys, any ideas for this problem in CI? Tks

Also, is there any problem setting debug level for the logging in test mode? I think we get more info from the code - I put some in init.

Copy link
Contributor Author

dpizetta commented Nov 19, 2019

@goanpeca could give some help with those issues on CI's, both are in Python2.7 and related to the return of a function that returns -1 when compared to 1 (-1 == 1). Thanks. Later I'll solve the conflicts, so you can review the code. It should be working without any drastic API changes.

Copy link
Contributor

goanpeca commented Nov 19, 2019

@goanpeca could give some help with those issues on CI's, both are in Python2.7 and related to the return of a function that returns -1 when compared to 1 (-1 == 1).

Sure! will do :-)

goanpeca added the v2.0 label Feb 19, 2020
goanpeca removed this from the v2.0 milestone Feb 19, 2020
Copy link
Contributor Author

dpizetta commented May 8, 2020

@goanpeca some new about this one? I think it is working properly, just the problem with the tests that seem not to be with the implementation here. If I could help more, please, let me know tks

goanpeca removed their assignment Aug 22, 2020
UmbralReaper mentioned this pull request May 3, 2021
ccordoba12 added this to the v2.0.0 milestone Aug 10, 2021
ccordoba12 removed the v2.0 label Aug 10, 2021
dalthviz removed this from the v2.0.0 milestone Nov 1, 2021
dalthviz added this to the v2.3.0 milestone Jul 15, 2022
CAM-Gerlach mentioned this pull request Sep 18, 2022
dalthviz modified the milestones: v2.3.0, v3.0.0 Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ccordoba12 ccordoba12 left review comments

+1 more reviewer

goanpeca goanpeca left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.0.0

Development

Successfully merging this pull request may close these issues.

4 participants