ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
124 stars 139 forks source link

Add conditional substitution #734

Closed nlamprian closed 11 months ago

nlamprian commented 11 months ago

Closes #727

clalancette commented 11 months ago

CI:

nlamprian commented 11 months ago

Fixed, allegedly. I did run the tests successfully last time. I am not able to reproduce the errors. I am working on a fresh ros:rolling-ros-core container. I haven't identified what's different from the logs.

clalancette commented 11 months ago

Fixed, allegedly. I did run the tests successfully last time. I am not able to reproduce the errors. I am working on a fresh ros:rolling-ros-core container. I haven't identified what's different from the logs.

Out of curiosity, what does pip3 list | grep flake8 in the container show you?

clalancette commented 11 months ago

Locally I'm still seeing:

_________________________________ test_flake8 __________________________________
test/test_flake8.py:23: in test_flake8
    assert rc == 0, \
E   AssertionError: Found 1 code style errors / warnings:
E     ./launch/substitutions/if_else_substitution.py:27:1: I100 Import statements are in the wrong order. 'from .substitution_failure import SubstitutionFailure' should be before 'from ..utilities import perform_substitutions'
E   assert 1 == 0
nlamprian commented 11 months ago
root@nlamprian:~/ros_ws# pip3 list | grep flake8
ament-flake8                         0.15.2
flake8                               4.0.1

The last error is hopefully fixed (the message can be misinterpreted).

clalancette commented 11 months ago
root@nlamprian:~/ros_ws# pip3 list | grep flake8
ament-flake8                         0.15.2
flake8                               4.0.1

OK, yeah. You are missing a bunch of plugins, and flake8 is a bit annoying in that you can't specify which ones are required; it only runs the plugins you have installed. My list looks like this:

ament-flake8                         0.15.2
flake8                               4.0.1
flake8-blind-except                  0.2.0
flake8-builtins                      1.5.3
flake8-class-newline                 1.6.0
flake8-comprehensions                3.8.0
flake8-deprecated                    1.3
flake8-docstrings                    1.6.0
flake8-import-order                  0.18.1
flake8-quotes                        3.3.1

We have a long-term plan to fix this up by making those plugins hard requirements, but we aren't there today.

Anyway, this looks good to me now (verified locally), so I'll run CI on this again. Thanks for iterating.

clalancette commented 11 months ago

CI:

federicociresola commented 4 months ago

@nlamprian is it possible to include this feature also in the Humble branch?