Skip to content
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 conditional substitution #734

Merged
merged 3 commits into from
Sep 14, 2023
Merged

Conversation

nlamprian
Copy link
Contributor

@nlamprian nlamprian commented Sep 12, 2023

Closes #727

Closes: ros2#727

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic to me, thanks for a nice clean pull request. I'll run CI on it next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nlamprian It looks like flake8 is unhappy here, mostly about double quotes but also about some import order stuff. I don't know why the Rpr job didn't pick up on it, but if you setup your workspace locally it should show it to you. That will need to be fixed before we merge this, thanks.

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
@nlamprian
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
@nlamprian
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 11c0b97 into ros2:rolling Sep 14, 2023
3 checks passed
@nlamprian nlamprian deleted the nlamprian/ifelse branch September 14, 2023 12:38
wjwwood pushed a commit that referenced this pull request Sep 20, 2023
* Add conditional substitution

Closes: #727

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
@federicociresola
Copy link

federicociresola commented Apr 17, 2024

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

ottojo pushed a commit to ottojo/launch that referenced this pull request Apr 25, 2024
* Add conditional substitution

Closes: ros2#727

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
@ottojo ottojo mentioned this pull request Apr 26, 2024
clalancette pushed a commit that referenced this pull request May 8, 2024
* Add conditional substitution

Closes: #727

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
Co-authored-by: Nick Lamprianidis <info@nlamprian.me>
jplapp pushed a commit to pixel-robotics/launch that referenced this pull request May 11, 2024
* Add conditional substitution

Closes: ros2#727

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
Co-authored-by: Nick Lamprianidis <info@nlamprian.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse arguments into their relevant type
3 participants