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

Implement #139 #140

Merged
merged 4 commits into from
Sep 21, 2016
Merged

Implement #139 #140

merged 4 commits into from
Sep 21, 2016

Conversation

mottosso
Copy link
Owner

@mottosso mottosso commented Sep 17, 2016

This introduces a radical change to counter #139, #109, #112 and #96.

The change involves moving all added members of Qt.py to it's own individual module, QtShim.

# Before
import Qt
Qt.setSectionResizeMode()
Qt.__binding__

# After
from Qt import QtShim
QtShim.setSectionResizeMode()
QtShim.__binding__ == "PyQt5"

It also counteracts our current Achilles heel, which is that at some point one of our additions are bound to conflict with native existing members of the binding. For example, if one day PySide decides to add __qt_version__ to their offering, but have a different type for it, like tuple, whereas we have str

So I'm on the fence about whether this is the right way to do it. The disadvantage is a more complex code base; the reader must now understand how self plays a part, and how QtShim even makes it into the Qt namespace.

So let me know what you think.

@fredrikaverpil
Copy link
Collaborator

fredrikaverpil commented Sep 18, 2016

This only applies to members we've added ourselves, right? (so for instance, QtWidgets is not changed but e.g. load_ui is moved?

Could we just make a list of them here before merging, for completeness?
I'm wondering if QtShim is the best name though.

@mottosso
Copy link
Owner Author

mottosso commented Sep 18, 2016

This only applies to members we've added ourselves, right? (so for instance, QtWidgets is not changed but e.g. load_ui is moved?

Yeah.

Remap as per usual, and make our own space for our own members.

Could we just make a list of them here before merging, for completeness?

Of course, list is in the README.

I'm wondering if QtShim is the best name though.

The name is up for grabs. QtShim is bad because it has no obvious connection to Qt.py. I wanted to name it Qt.py, but of course couldn't. QtCompat was another option that crossed my mind, but wasn't sure since load_ui isn't necessarily a compatibility function. Got anything in mind?

Edit: Not to mention __binding__ and similar.

@fredrikaverpil
Copy link
Collaborator

fredrikaverpil commented Sep 18, 2016

Of course, list is in the README.

Ah, excellent. Makes it easier to come up with a good name for this.

Hm. How about something like

from Qt import Custom

I mean.. what are the chances that... hm. I'll have to sleep on it.

@mottosso
Copy link
Owner Author

How was your sleep? :)

How about..?

# After
from Qt import QtCompat
QtCompat.setSectionResizeMode()
QtCompat.__binding__ == "PyQt5"

I think we'll need the Qt-prefix on whichever name we choose, to make it blend better with other calls. In the middle of a block of code, it can be difficult to tell where Custom or something else without this prefix comes from.

button = QtWidgets.QPushButton("Button")
Custom.someOtherFunction()  # Is this related to Qt?

@fredrikaverpil
Copy link
Collaborator

fredrikaverpil commented Sep 20, 2016

from Qt import QtCompat

Yep. Sounds good. Let's go for it.

Does this backwards-breaking feature mean that we should also scramble for a v1.0.0 release?

@mottosso
Copy link
Owner Author

mottosso commented Sep 20, 2016

It doesn't break backwards compatibility, the current members still work. Once we hit 1.0, we can remove them.

See the _maintain_backwards_compatibility function at the bottom.

This points out a potential problem with a previous fix for compatibility with https://github.com/pyqt/python-qt. Before, these members were tested against existing members, but now they are blindly added via `setattr`. It's safe, since we've already tested these and know for sure that they aren't being overwritten. But it still opens the door for these to one day override a newly added member.

Luckily, this is only relevant until the next major release, upon which they are all discarded.
@fredrikaverpil
Copy link
Collaborator

See the _maintain_backwards_compatibility function at the bottom.

Ah, that's perfect.

Once we hit 1.0, we can remove them.

Sounds good!

This all looks excellent to me.

@mottosso mottosso merged commit 8f3ed0e into mottosso:master Sep 21, 2016
@mottosso mottosso deleted the implement139 branch March 24, 2017 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants