addressing Dirk's feedback on the rqt_py_common port #155

mlautman · 2018-11-08T01:43:50Z

This cleans up some rqt_py_common code and adds the get_field_type function call

dirk-thomas

Please don't use blank noqa. Either ignore specific warnings in very individual cases or if you don't want make the package follow the style guide disable the linter. Adding blank noqa's all over the place isn't helpful.

dirk-thomas · 2018-11-09T23:18:25Z

rqt_py_common/package.xml

@@ -33,8 +33,6 @@
  <test_depend>ament_lint_auto</test_depend>
  <test_depend>ament_lint_common</test_depend>
  <test_depend>ament_cmake_pytest</test_depend>
-  <test_depend>ament_flake8</test_depend>
-  <test_depend>ament_pep257</test_depend>
  <test_depend>python3-pytest</test_depend>


Same comment: python3-pytest doesn't seem to be used anywhere directly.

mlautman · 2018-11-10T02:44:53Z

@dirk-thomas I am only putting #noqa on existing lines of code. It would take too much time to go through each flake8 warning in the repo and fix or disable the exact warnings. All new lines of code adhere to strict flake8.

brawner · 2018-11-13T00:02:52Z

If you want to skip flake8 tests on a file, you can place the following at the top of your file

# flake8: noqa

dirk-thomas

Please don't use blank noqa.

I am only putting #noqa on existing lines of code.

Please only suppress specific warnings, e.g. # noqa: D102.

mlautman · 2018-11-13T22:24:49Z

@dirk-thomas I removed the # noqa's as suppressing specific warnings would take significant time.

mlautman · 2018-11-13T23:04:27Z

https://ci.ros2.org/job/ci_linux/5536/

dirk-thomas · 2018-11-14T22:56:22Z

rqt_py_common/src/rqt_py_common/plugin_container_widget.py

        @type sysmsg: str
        """
        self._logger.info('{}'.format(sysmsg))
        # self._sysmsg_widget.setPlainText(sysmsg)
        self._sysmsg_widget.append(sysmsg)

    def shutdown(self):
-
+        """PluginContainerWidget.shutdown."""


This docblock has no added value beyond the already known class name and method name. Please use # noqa: D102 instead if there is no documentation.

Same for similar cases below.

dirk-thomas · 2018-11-14T22:57:27Z

rqt_py_common/src/rqt_py_common/topic_dict.py

+        """
+        update_topics.
+
+        Update the topics contained in the dictionary with new information from a node


This line is valuable but please remove the two line above.

Also add a period

dirk-thomas · 2018-11-14T22:59:26Z

rqt_py_common/src/rqt_py_common/topic_helpers.py

@@ -70,106 +77,173 @@ def get_topic_names_and_types(node=None):


 _message_class_cache = {}
-def get_message_class(message_type):
-    logger = logging.get_logger("get_message_class")
+def get_message_class(message_type):  # noqa: C901 E302


You don't need E302 here. Simply at two empty lines between the cache variable and the function definition.

dirk-thomas · 2018-11-14T22:59:29Z

rqt_py_common/src/rqt_py_common/topic_helpers.py

-    logger = logging.get_logger("get_message_class")
+def get_message_class(message_type):  # noqa: C901 E302
+    """
+    get_message_class: gets the message class from a string representation.


No need to repeat the function name:

Get the message class from a string representation.

brawner · 2018-11-15T16:52:50Z

rqt_py_common/src/rqt_py_common/topic_dict.py

+        """
+        TopicDict(node).
+
+        Create a Topic Dict with an option node passed in


Add a period and use this line as the summary

brawner · 2018-11-15T16:53:13Z

rqt_py_common/src/rqt_py_common/topic_dict.py

+        """
+        get_topics.
+
+        Get the topic dictionary


Use this as the summary line on a single line docstring.

brawner · 2018-11-15T16:53:27Z

rqt_py_common/src/rqt_py_common/topic_dict.py

+        """
+        update_topics.
+
+        Update the topics contained in the dictionary with new information from a node


Also add a period

brawner · 2018-11-15T16:55:22Z

rqt_py_common/src/rqt_py_common/topic_helpers.py

        return float

-    elif type_name in ['string']:
-        # string constants are always stripped
+    # TODO(mlautman): char should be a string of lenght one. We do not currently suppor this


rqt_py_common/src/rqt_py_common/topic_helpers.py

brawner · 2018-11-15T16:56:59Z

rqt_py_common/test/test_rqt_roscomm_util.py

        unittest.TestCase.setUp(self)

-    def tearDown(self):
+    def tearDown(self):  # noqa
        unittest.TestCase.tearDown(self)
        # del self._model

    # TODO(mlautman): Replace this test with an apropriate ROS2 test.


Spelling

Suggested change

# TODO(mlautman): Replace this test with an apropriate ROS2 test.

# TODO(mlautman): Replace this test with an appropriate ROS2 test.

brawner · 2018-11-15T16:57:52Z

rqt_py_common/test/test_rqt_roscomm_util.py

        unittest.TestCase.tearDown(self)
        # del self._model

    # TODO(mlautman): Replace this test with an apropriate ROS2 test.
    def test_iterate_packages(self):
-        """
+        """  # noqa


Just make this a single line docstring and add a period.

mlautman · 2018-11-16T18:59:27Z

@dirk-thomas @brawner Changes made. Merge?

brawner

Looks pretty good to me, just a couple more changes

brawner · 2018-11-19T21:16:48Z

rqt_py_common/CMakeLists.txt

-  # Disabling copyright test. The copyright used in this package does not conform to
-  # ament's copyright tests
-  set(ament_cmake_copyright_FOUND TRUE)
-  ament_lint_auto_find_test_dependencies()


If you remove the auto tests here, you can remove the find_package line above.

As well as the test dependencies in the manifest.

brawner · 2018-11-19T21:17:55Z

rqt_py_common/src/rqt_py_common/message_tree_model.py

-        # TODO(mlautman): Work around missing _slot_types in new msg types
-        if hasattr(slot, '__slots__') and hasattr(slot, '_slot_types'):
-            for child_slot_name, child_slot_type in zip(slot.__slots__, slot._slot_types):
+        if hasattr(slot, '__slots__') and hasattr(slot, 'get_fields_and_field_types'):


If you check for get_fields_and_field_types you can remove the check for '__slots__', since that's not used here.

rqt_py_common/src/rqt_py_common/plugin_container_widget.py

brawner · 2018-11-19T21:21:27Z

rqt_py_common/src/rqt_py_common/topic_dict.py

 class TopicDict(object):
+    """TopicDict class."""


Remove docstring, if you don't want a useful summary

mlautman · 2018-11-19T21:45:35Z

@brawner feedback addressed

rqt_py_common/src/rqt_py_common/topic_helpers.py

dirk-thomas · 2018-11-19T22:12:49Z

rqt_py_common/CMakeLists.txt

-  # Disabling copyright test. The copyright used in this package does not conform to
-  # ament's copyright tests
-  set(ament_cmake_copyright_FOUND TRUE)
-  ament_lint_auto_find_test_dependencies()


As well as the test dependencies in the manifest.

rqt_py_common/src/rqt_py_common/topic_helpers.py

dirk-thomas · 2018-11-19T22:20:00Z

rqt_py_common/src/rqt_py_common/topic_helpers.py

+    logger = logging.get_logger('topic_helpers._get_field_type')
+    for name, types in topic_names_and_types:
+        # If the topic is a substring of the target param, then we have found a match
+        if target.find(name) >= 0:


I don't understand why target.find(name) > 0 would be a match? In the following code you expect target == name or use it as target[len(name):].

I changed this line of logic to target.startswith(name)

The updated condition still seems to match unrelated ones. E.g. target being /foobar/baz and name being /foo.

Good point. New commit covers that corner case and tests for it as well

rqt_py_common/src/rqt_py_common/topic_helpers.py

mlautman · 2018-11-19T23:59:22Z

@dirk-thomas comments addressed.

mlautman · 2018-11-20T02:35:33Z

This was blocking both me and @brawner. After many rounds of reviews, I am merging it with only one approval

mlautman requested review from brawner and dirk-thomas November 8, 2018 01:43

ghost assigned mlautman Nov 8, 2018

ghost added the in progress label Nov 8, 2018

mlautman force-pushed the rqt-py-common-fixup branch from ac3e30a to 15c9078 Compare November 8, 2018 02:07

brawner mentioned this pull request Nov 9, 2018

rqt_py_common port #148

Merged

dirk-thomas reviewed Nov 9, 2018

View reviewed changes

mlautman mentioned this pull request Nov 10, 2018

Get field types #158

Closed

mlautman force-pushed the rqt-py-common-fixup branch from 3be2dc8 to 668bd22 Compare November 10, 2018 03:38

dirk-thomas suggested changes Nov 13, 2018

View reviewed changes

mlautman force-pushed the rqt-py-common-fixup branch from 80d9076 to 5e37963 Compare November 13, 2018 22:23

mlautman force-pushed the rqt-py-common-fixup branch 3 times, most recently from 0cf24aa to a93b7c4 Compare November 13, 2018 23:00

mlautman mentioned this pull request Nov 13, 2018

Rqt daemon node #159

Closed

dirk-thomas reviewed Nov 14, 2018

View reviewed changes

brawner requested changes Nov 15, 2018

View reviewed changes

mlautman force-pushed the rqt-py-common-fixup branch 3 times, most recently from 692a99d to d2cd844 Compare November 15, 2018 18:15

Mike Lautman added 5 commits November 16, 2018 18:17

addressing Dirk's feedback on the rqt_py_common port

f52cf0d

flake8

884453d

idl matching

58c7262

fixup! flake8

368a49d

Making changes to get_topic_names_and_types

0001c1e

Mike Lautman added 6 commits November 16, 2018 18:20

fixup! flake8

34d99d6

implementing get_field_types

79148f2

removing CATKIN_IGNORE

e3ac5ec

removing noqa

9f14976

pr fixup

c8df3e1

slot types

e99f6cb

mlautman force-pushed the rqt-py-common-fixup branch from b07d579 to ccc9fe3 Compare November 17, 2018 01:21

mlautman mentioned this pull request Nov 19, 2018

Rqt py common message helpers #162

Merged

mlautman force-pushed the rqt-py-common-fixup branch from ccc9fe3 to 40ba30a Compare November 19, 2018 21:09

fixup to match changes to rosidl_python

bf6f31f

mlautman force-pushed the rqt-py-common-fixup branch from 40ba30a to bf6f31f Compare November 19, 2018 21:10

brawner reviewed Nov 19, 2018

View reviewed changes

pr fixup

33cd6bf

mlautman force-pushed the rqt-py-common-fixup branch from 37de6b5 to 33cd6bf Compare November 19, 2018 21:45

brawner approved these changes Nov 19, 2018

View reviewed changes

dirk-thomas mentioned this pull request Nov 19, 2018

ignoring unported packages #157

Merged

dirk-thomas reviewed Nov 19, 2018

View reviewed changes

addressing Dirk's comments

db77038

mlautman force-pushed the rqt-py-common-fixup branch from bc055c2 to db77038 Compare November 20, 2018 00:08

fixing get_field_type

7675554

mlautman merged commit fadface into crystal-devel Nov 20, 2018

ghost removed the in progress label Nov 20, 2018

mlautman deleted the rqt-py-common-fixup branch November 20, 2018 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

addressing Dirk's feedback on the rqt_py_common port #155

addressing Dirk's feedback on the rqt_py_common port #155

	# TODO(mlautman): Replace this test with an apropriate ROS2 test.
	# TODO(mlautman): Replace this test with an appropriate ROS2 test.

addressing Dirk's feedback on the rqt_py_common port #155

addressing Dirk's feedback on the rqt_py_common port #155

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment