Ros2 port by brawner · Pull Request #52 · ros-visualization/python_qt_binding

brawner · 2018-10-20T00:45:22Z

This ports python_qt_binding to work with ros2 crystal. I've also added tests to ensure pyqt or pyside are installed correctly and the package works as intended.

… into pep8

* autopep8 * wrap * autopep8 fixup

brawner · 2018-10-20T00:49:31Z

Link to test
https://ci.ros2.org/job/ci_linux/5305/

brawner · 2018-10-20T07:13:59Z

Ok, CI build failed. I have some work to do I guess.

mlautman · 2018-10-23T18:00:30Z

@brawner Ping me when this is ready to be reviewed

dirk-thomas

When moving / renaming existing file (like python_qt_binding/binding_helper.py) please use a separate commit for that - that will "help" git to track the history of the file.

dirk-thomas · 2018-10-24T21:34:22Z

CMakeLists.txt

-catkin_python_setup()
+ament_python_install_package(${PROJECT_NAME})

+set(python_qt_binding_CMAKE_DIRS cmake)


Where is this variable being used?

It's not, removed.

dirk-thomas · 2018-10-24T21:34:34Z

CMakeLists.txt

  cmake/sip_configure.py
  cmake/sip_helper.cmake
-  DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}/cmake)
+  DESTINATION share/${PROJECT_NAME}/cmake/)


No trailing slash.

dirk-thomas · 2018-10-24T21:35:29Z

CMakeLists.txt

+    ament_lint_auto_find_test_dependencies()
+
+    ament_add_pytest_test(pqt_test test
+      PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}"


This is the default value so it doesn't need to be passed explicitly.

dirk-thomas · 2018-10-24T21:36:18Z

CMakeLists.txt

+
+    ament_add_pytest_test(pqt_test test
+      PYTHON_EXECUTABLE "${PYTHON_EXECUTABLE}"
+      APPEND_ENV AMENT_PREFIX_PATH=${ament_index_build_path}


Where is the variable ament_index_build_path being defined?

Leftover from a copy-paste job. Removed append to AMENT_PREFIX_PATH

dirk-thomas · 2018-10-24T21:37:37Z

LICENSE

@@ -0,0 +1,202 @@
+
+                                 Apache License


The existing code is licensed under BSD. You can't change the license of it without an agreement of all contributors.

Whoops, sorry! Trying to pass test_copyright.py tests, but I had removed that awhile ago.

dirk-thomas · 2018-10-24T21:40:45Z

python_qt_binding/__init__.py

+from python_qt_binding.binding_helper import \
+    loadUi, QT_BINDING, QT_BINDING_MODULES, QT_BINDING_VERSION
+
+print('Qt5 Python Binding {}: Version {}'.format(QT_BINDING, QT_BINDING_VERSION))


This message was probably used for debugging and should be removed again.

This line was added, as well as the loadUI = loadUi so that flake8 would not complain about unused variables. Because the init exposes loadUI, QT_BINDING and QT_BINDING_VERSION, but doesn't use them in the file flake8 thinks they are unused.

@dirk-thomas Do you have a recommended resolution?

You can suppress a specific flake8 warning by appending # noqa: X123 to the line.

Adding noqa: F401 to suppress unused imports. I guess it's recommended to add these to an all variable, but I'm not sure how that would work with the dynamic module imports associated with this package.

dirk-thomas · 2018-10-24T21:41:02Z

python_qt_binding/__init__.py

+    loadUi, QT_BINDING, QT_BINDING_MODULES, QT_BINDING_VERSION
+
+print('Qt5 Python Binding {}: Version {}'.format(QT_BINDING, QT_BINDING_VERSION))
+loadUi = loadUi


What is this line about?

Removed in favor of noqa: F401

dirk-thomas · 2018-10-24T21:41:51Z

python_qt_binding/binding_helper.py

@@ -1,5 +1,3 @@
-#!/usr/bin/env python


Please keep this line. It allows to invoke the file directly to test the selected binding.

Reverting this line

dirk-thomas · 2018-10-24T21:43:22Z

python_qt_binding/binding_helper.py


 def loadUi(uifile, baseinstance=None, custom_widgets=None):
    """
+    Load a provided UI file using PySide2.


The reference of PySide2 is incorrect. This function uses the selected binding.

dirk-thomas · 2018-10-24T21:44:42Z

setup.py


 try:
-    from setuptools import setup
+    from setuptools import setup, find_packages


In the case of an exception the symbol find_packages isn't defined and would therefore fail later.

This was part of debugging, reverting to the old setup.py

dirk-thomas · 2018-10-24T22:19:09Z

Also moving the Python package out of the src folder would be good to either apply in the "upstream" ROS 1 branch or not do it on the ROS 2 branch - since it makes porting patches between them more difficult.

brawner · 2018-10-24T23:34:18Z

@dirk-thomas from your comments on slack, I found how to set the PACKAGE_DIR argument with ament_python_install_package. Moving files back to src

brawner · 2018-10-25T17:24:44Z

CI build and tests passed
https://ci.ros2.org/job/ci_linux/5343/

dirk-thomas · 2018-10-25T17:26:16Z

CMakeLists.txt

-  DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION}/cmake)
+  DESTINATION share/${PROJECT_NAME}/cmake)
+
+  if(BUILD_TESTING)


Everything from here shouldn't be indented.

dirk-thomas · 2018-10-25T17:27:50Z

CMakeLists.txt

+    find_package(ament_lint_auto REQUIRED)
+    ament_lint_auto_find_test_dependencies()
+
+    ament_add_pytest_test(pqt_test test


pqt_test - hat does pqt stand for?

dirk-thomas · 2018-10-25T17:28:16Z

CONTRIBUTING.md

+   Notwithstanding the above, nothing herein shall supersede or modify
+   the terms of any separate license agreement you may have executed
+   with Licensor regarding such Contributions.
+~~~


This is still mentioning the wrong license.

dirk-thomas · 2018-10-25T17:28:54Z

cmake/python_qt_binding-extras.cmake

@@ -0,0 +1,2 @@
+# location of cmake files in installspace
+set(python_qt_binding_EXTRAS_DIR "${python_qt_binding_DIR}")


If the variable isn't used anywhere (in downstream packages?) then it can be removed.

dirk-thomas · 2018-10-25T17:33:26Z

I syncronized the crystal-devel branch to contain the latest state of kinetic-devel so this needs to be rebased.

…nding into ros2_port

dirk-thomas

LGTM

brawner and others added 13 commits October 12, 2018 16:59

Modifying files for ros2

31dccab

Moving .cmake.em to .cmake

f94df1b

Removing QtBindingHelper

a691c80

autopep8

261fe3b

Merge branch 'pep8' of github.com:ros-visualization/python_qt_binding…

0f985d3

… into pep8

Merge branch 'remove_QtBindingHelper' into ros2_port

e5d8149

Adding tests

3793fd0

autopep8 (#51)

287a3e3

* autopep8 * wrap * autopep8 fixup

Adding tests

055ec1d

Merging

40f9999

Reverting copyright

6aa914f

Updating package.xml

850ee2a

Cleaning up

d3483f6

brawner requested review from dirk-thomas and mlautman October 20, 2018 00:45

ghost assigned brawner Oct 20, 2018

ghost added the in progress label Oct 20, 2018

brawner added 11 commits October 22, 2018 12:16

Updating package.xml

2141af9

Cmakelists

9ee167e

package.xml

852e4af

tests working again

39cef64

package.xml

7d21f36

package.xml

0aaa9c4

package.xml

f0039db

package.xml

a1c2de0

package.xml

d3635f4

package.xml

e421a4d

package.xml

edbd6b1

brawner added 3 commits October 22, 2018 20:49

package.xml

5bd6c7e

cmakelists

9f277dc

Updating cmake files to work with qt_gui_cpp

10115d0

brawner added 4 commits October 23, 2018 18:47

Building and installing

706e6f1

Reverting to original imports

1ed29f6

Pass flake8

6009e41

Removing install

ba5991c

dirk-thomas reviewed Oct 24, 2018

View reviewed changes

brawner added 2 commits October 24, 2018 16:10

Moving python files back to src

822a5d0

Addressing pull request

3d3d2b6

brawner added 2 commits October 24, 2018 16:51

Addressing pull request comments

9cf28ff

Cleaning up

3aadc29

dirk-thomas reviewed Oct 25, 2018

View reviewed changes

one import per line

2a393aa

ghost assigned dirk-thomas Oct 25, 2018

brawner and others added 4 commits October 25, 2018 14:21

Addressing pull request comments

022276b

Merge branch 'ros2_port' of github.com:ros-visualization/python_qt_bi…

c01294e

…nding into ros2_port

Merging from crystal-devel

a937123

comment case only

168fd0f

dirk-thomas approved these changes Oct 25, 2018

View reviewed changes

mlautman merged commit a527d32 into crystal-devel Oct 26, 2018

ghost removed the in progress label Oct 26, 2018

mlautman deleted the ros2_port branch October 26, 2018 00:07

		@@ -0,0 +1,2 @@
		# location of cmake files in installspace
		set(python_qt_binding_EXTRAS_DIR "${python_qt_binding_DIR}")

Conversation

brawner commented Oct 20, 2018

Uh oh!

brawner commented Oct 20, 2018

Uh oh!

brawner commented Oct 20, 2018

Uh oh!

mlautman commented Oct 23, 2018

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Oct 24, 2018

Uh oh!

brawner commented Oct 24, 2018

Uh oh!

brawner commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dirk-thomas commented Oct 25, 2018

Uh oh!

dirk-thomas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone