Skip to content

Add conditional substitution#734

Merged
clalancette merged 3 commits intoros2:rollingfrom
nlamprian:nlamprian/ifelse
Sep 14, 2023
Merged

Add conditional substitution#734
clalancette merged 3 commits intoros2:rollingfrom
nlamprian:nlamprian/ifelse

Conversation

@nlamprian
Copy link
Copy Markdown
Contributor

@nlamprian nlamprian commented Sep 12, 2023

Closes #727

Closes: ros2#727

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
Copy link
Copy Markdown
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
Copy Markdown
Contributor

CI:

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

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

CI:

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

@clalancette clalancette merged commit 11c0b97 into ros2:rolling Sep 14, 2023
@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
Copy Markdown

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>
emersonknapp pushed a commit to emersonknapp/launch that referenced this pull request May 1, 2025
* Add conditional substitution

Closes: ros2#727

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
emersonknapp pushed a commit to emersonknapp/launch that referenced this pull request May 19, 2025
* Add conditional substitution

Closes: ros2#727

Signed-off-by: Nick Lamprianidis <info@nlamprian.me>
christophebedard pushed a commit that referenced this pull request May 29, 2025
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