fix lint tests #14
fix lint tests #14
Conversation
|
dealt with in #13 |
|
Since @wjwwood removed the patch from his PR (#12 (comment)) this can be reopened. |
| # Remove implementations that are being filtered for in the rclpy CMakeLists so | ||
| # that they cannot be selected | ||
| __rmw_implementations = [rmw_impl for rmw_impl in __rmw_implementations | ||
| if rmw_impl not in ['rmw_connext_dynamic_cpp', 'rmw_fastrtps_cpp']] |
There was a problem hiding this comment.
For PEP8 compliance can you modify the wrapping and indentation to:
__rmw_implementations = [
rmw_impl for rmw_impl in __rmw_implementations
if rmw_impl not in ['rmw_connext_dynamic_cpp', 'rmw_fastrtps_cpp']]
|
We can certainly merge this before mine. +1 (with @dirk-thomas's comments addressed). |
|
ok fixed. Sorry about this pep8 stuff - something is missing on my system so that the lint checks are just spitting out junk (around 1000 lines just for |
| .format(__selected_rmw_implementation, get_rmw_implementations()) | ||
| ) | ||
| raise InvalidRCLPYImplementation() | ||
| ''' |
| @@ -85,5 +88,4 @@ def import_rmw_implementation(): | |||
| module_name = '._rclpy__{rmw_implementation}'.format( | |||
| rmw_implementation=__selected_rmw_implementation, | |||
| ) | |||
| __rmw_implementation_module = importlib.import_module(module_name, package='rclpy') | |||
There was a problem hiding this comment.
I think you dropped this one unintentionally? 😛
There was a problem hiding this comment.
ah, you guys are too fast (and I have not been paying close enough attention - sorry for wasting your time on this.)
There was a problem hiding this comment.
No worries, we're supposed to wait until it is in review (i.e. it has the in review tag) anyways 😄.
| @@ -30,8 +30,11 @@ def reload_rmw_implementations(): | |||
| global __rmw_implementations | |||
| __rmw_implementations = sorted(ament_index_python.get_resources('rmw_implementation').keys()) | |||
|
|
|||
| # Remove implementations that are being filtered for in the rclp CMakeLists so they cannot be selected | |||
| __rmw_implementations = [rmw_impl for rmw_impl in __rmw_implementations if rmw_impl not in ['rmw_connext_dynamic_cpp', 'rmw_fastrtps_cpp']] | |||
| # Remove implementations that are being filtered for in the rclpy CMakeLists so | |||
There was a problem hiding this comment.
Nitpick: I would wrap after CMakeLists then the next line makes more sense on its own.
There was a problem hiding this comment.
thank you, I'll do that from now on
|
+1 |
|
lgtm too |
|
Regarding my previous comment re linter tests failing, I found (in addition to ament/ament_lint#48) that in the CI build there's a line which is (mostly) missing from the source installation instructions, so I was missing some of the packages. Since the instructions call ament with |
|
@dhood I think you're right, it appears to be missing from both the Linux and OS X development guides. Ironically I think the Windows setup guide is up-to-date: https://github.com/ros2/ros2/wiki/Windows-Development-Setup#installing-a-few-dependencies It's probably because our Linux and OS X machines are already set up with these tools for our other work, whereas the Windows machines are otherwise unused. Can you update those wiki pages (Linux and OS X)? If you need help or want a review on your changes, just let me know. |
No description provided.