-
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
Add pyside2-uic (executable) and pyside2uic (module) #142
Add pyside2-uic (executable) and pyside2uic (module) #142
Conversation
I realize I need to add a test to this. Will try to do that later this evening. |
xvfb | ||
|
||
# Make pyside2uic availble for Python 2.x | ||
RUN cp -avr /usr/lib/python3/dist-packages/pyside2uic /usr/local/lib/python2.7/dist-packages |
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.
Why are the tools installed for Python 3? :O
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 really don't know. I asked the author of the pyside2-tools apt package the same thing.
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.
Okay, well that's fine. This solution should work, they're Python 2 compatible at least?
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.
That I don't know yet (but assume so). Let me try it out via tests.
I added a test which compiles an the already existing ui in tests.py (qwidget.ui) into a .py file (output.py): # -*- coding: utf-8 -*-
# Form implementation generated from reading ui file 'qwidget.ui'
#
# Created: Sun Sep 18 13:22:20 2016
# by: pyside2-uic running on PySide2 2.0.0~alpha0
#
# WARNING! All changes made in this file will be lost!
from PySide2 import QtCore, QtGui, QtWidgets
class Ui_Form(object):
def setupUi(self, Form):
Form.setObjectName("Form")
Form.resize(235, 149)
self.gridLayout = QtWidgets.QGridLayout(Form)
self.gridLayout.setObjectName("gridLayout")
self.lineEdit = QtWidgets.QLineEdit(Form)
self.lineEdit.setObjectName("lineEdit")
self.gridLayout.addWidget(self.lineEdit, 0, 0, 1, 1)
self.retranslateUi(Form)
QtCore.QMetaObject.connectSlotsByName(Form)
def retranslateUi(self, Form):
Form.setWindowTitle(QtWidgets.QApplication.translate("Form", "Form", None, -1)) |
I'm not sure on how to assert this functionality. Any suggestions? |
Compile something you are familiar with and know the outcome of. Then just Have a look at this for example. |
Jotting down this here since I can't find anything on the def compileUi(uifile, pyfile, execute=False, indent=4, from_imports=False):
"""compileUi(uifile, pyfile, execute=False, indent=4, from_imports=False)
Creates a Python module from a Qt Designer .ui file.
uifile is a file name or file-like object containing the .ui file.
pyfile is the file-like object to which the Python code will be written to.
execute is optionally set to generate extra Python code that allows the
code to be run as a standalone application. The default is False.
indent is the optional indentation width using spaces. If it is 0 then a
tab is used. The default is 4.
from_imports is optionally set to generate import statements that are
relative to '.'.
""" |
The problem is I've never compiled anything before so I'm not familiar with what the output should be like. In this case, I think we should be able to assume the output is proper and that it all works. But I'd like someone to confirm that. |
@@ -311,6 +311,28 @@ def test_import_from_qtwidgets(): | |||
assert QPushButton.__name__ == "QPushButton", QPushButton | |||
|
|||
|
|||
def test_pyside2uic(): |
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.
Wait, what are we testing here? Are we testing Qt.py, or are we testing pyside2-tools?
We only need to test what we add to the table. In this case, python -m Qt --compile my_ui.ui
and the --convert
option.
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'm just testing pyside2uic generally. You can compile via it so I figured that was a good test. I'm not testing Qt.py's --compile
or --convert
.
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.
Should I just remove my test then?
This PR was just all about adding pyside2-tools to the Dockerfiles.
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.
There's no need to include test for this. We can expect the PySide team to test their own tools.
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.
...since I was copying pyside2uic from Python 3 to Python 2 I figured it would be good to test that. Perhaps that should've been more clear in my test.
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.
Ah, but I think that's testing the Docker image within which we run tests for Qt.py. The tests don't belong amongst tests for Qt.py. We're basically producing tests for this particular distribution of pyside2-tools that we got from some random PPA for this particular OS. It doesn't prove anything. We should make sure what we install to run our tests are correct, and do that external from these tests.
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.
Ok, I'll just remove the test then. And we should be good to go and merge this if you feel it looks allright.
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.
It would be allright, but the addition you made, is it still uncertain whether or not that works?
We should find that out first, so we know whether we can rely on that the errors it gives us is related to Qt.py, and not the way we installed it.
I could do a hard reset to my first commit: 410d1f8 or we could just squash merge this as-is. |
It's an option, but we would lose out on the conversation. No harm one way or the other, I'll leave it up to you. |
I'll just merge this :) |
Fix #141