-
Notifications
You must be signed in to change notification settings - Fork 252
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
PyQt enums patch #352
base: master
Are you sure you want to change the base?
PyQt enums patch #352
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, good job with the little table there demonstrating the changes. Nice touch. 👍
However, this is too magical/automatic with too much formatting going on to understand what's happening. See comments in code.
If the goal is adding these two members, it would be much preferred just adding them directly, although it can't modify the original class so it'll need to go in QtCompat
# Bad
from PyQt4 import QtCore
QtCore.Qt.CheckState.Checked
# How did you get there?
# Ah.. someone has imported Qt.py somewhere
# Good
from Qt import QtCompat
QtCompat.CheckState.Checked
qt_attrs = { | ||
name: getattr(Qt.QtCore.Qt, name) | ||
for name in dir(Qt.QtCore.Qt) | ||
if not name.startswith("_") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one for example could (and likely is) pick up enums other than the 2 being fixed by this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it was the original intention to be able to capture all cases found, not just CheckState
only. The description in the PR could probably do with a better wording to highlight it.
tests.py
Outdated
values_path = "QtCore.Qt.{0}.{1}".format(class_name, "values") | ||
assert hasattr(qt_class, "values"), ( | ||
"{0} should exist, but is missing".format(values_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very hard to read
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to %s
notations in newest commits
Great point, I think the |
0fa580a: it's starting to gets messy imo, but I want to get an opinion on this. QtCompatAs per #352 (comment), the above implementation will work, but it does add the enums directly and if not careful, can conflict with existing attributes already inside EnumFlags classTo replicate the Bottom line, it works, but at what cost? |
Thanks for the update, but this is still to magical. Or implicit, is the word. Let's just make it real explicit instead, e.g. here: Lines 1547 to 1553 in 8a8953b
One could put.. CheckState = type("CheckState", (object,), {})
CheckState.Checked = Qt.Checked
CheckState.values = ... # etc
QtCompat.CheckState = CheckState
QtCompat.Enum2 = Qt.Enum2
QtCompat.Enum3 = Qt.Enum3
... # etc And so forth. That way, we control which enums are actually patched, and also document what they are. That implicit method is not only hard to read, but doesn't tell you which enums are patched. It also runs the risk of patching different enums on different builds of PySide2/PyQt5/etc. |
Also keep an eye on the tests, they're unhappy about something. |
Patch
PyQt*
Qt namespace enums to behave likePySide*
.i.e. Fixes ❎ in
QtCore.Qt
PyQt*
PySide*
Qt.Checked
valueQt.CheckState
classQt.CheckState.Checked
valueQt.CheckState.values
dictUPDATE Clarification: Above is just 1 example, the intention is to patch for all flags/enums in the Qt Namespace
Would this fall under a similar re-name/re-expose
pyqtSignal
asSignal
? Just curious if this will actually fall under the umbrella of No wrappers