Skip to content

Allow SomeSubstitutionsType in variable_name of LaunchConfiguration substitution#235

Merged
ivanpauno merged 1 commit intomasterfrom
ivanpauno/launch-config-allow-substitutions-in-name
May 15, 2019
Merged

Allow SomeSubstitutionsType in variable_name of LaunchConfiguration substitution#235
ivanpauno merged 1 commit intomasterfrom
ivanpauno/launch-config-allow-substitutions-in-name

Conversation

@ivanpauno
Copy link
Copy Markdown
Member

@ivanpauno ivanpauno commented May 13, 2019

Found this while working in launch_frontend.
I don't see a reason for not allowing SomeSubstitutionsType there.

@ivanpauno
Allow substitution in variable_name of LaunchConfiguration substitutions
65873c9 
Signed-off-by: ivanpauno <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label May 13, 2019
@ivanpauno ivanpauno requested a review from hidmic May 13, 2019 16:15
@ivanpauno ivanpauno self-assigned this May 13, 2019
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented May 13, 2019 •
edited
Loading

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status (known unrelated failures)
  • macOS Build Status (failures that seem to be unrelated to me)
  • Windows Build Status (known unrelated failures)

@ivanpauno ivanpauno mentioned this pull request May 13, 2019
Merged
@ivanpauno ivanpauno added the bug Something isn't working label May 15, 2019
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented May 15, 2019

Added bug label, because I think not allowing SomeSubstitutionsType was a leftover of an old change.

hidmic
hidmic approved these changes May 15, 2019
View reviewed changes
Copy link
Copy Markdown

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM pending green CI

launch/launch/substitutions/launch_configuration.py
"""Constructor."""
super().__init__()

ensure_argument_type(variable_name, str, 'variable_name', 'LaunchConfiguration')
Copy link
Copy Markdown

@hidmic hidmic May 15, 2019

Choose a reason for hiding this comment

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

@ivanpauno shouldn't we ensure it's SomeSubstitutionsType anyways?

Copy link
Copy Markdown
Member Author

@ivanpauno ivanpauno May 15, 2019

Choose a reason for hiding this comment

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

We aren't usually ensuring that arguments are of an specific type. e.g.:

launch/launch/launch/substitutions/environment_variable.py

Lines 26 to 44 in d2ea92c

class EnvironmentVariable(Substitution):
"""
Substitution that gets an environment variable value as a string.
If the environment variable is not found, it returns empty string.
"""
def __init__(
self,
name: SomeSubstitutionsType,
*,
default_value: SomeSubstitutionsType = ''
) -> None:
"""Constructor."""
super().__init__()
from ..utilities import normalize_to_list_of_substitutions # import here to avoid loop
self.__name = normalize_to_list_of_substitutions(name)
self.__default_value = normalize_to_list_of_substitutions(default_value)

I don't know why we should do it here and not in other places.

CI is already green or with unrelated errors. Could you please check macOS failures too? They seem to be unrelated.

Copy link
Copy Markdown

@hidmic hidmic May 15, 2019

Choose a reason for hiding this comment

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

Alright, fair enough.

@hidmic
Copy link
Copy Markdown

hidmic commented May 15, 2019

Test failures on Mac OS look like the message_filters ones, though I don't know if those are supposed to be fixed.

@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented May 15, 2019

MacOS faliures were unrelated (see ros2/build_farmer#189). Merging

@ivanpauno ivanpauno merged commit b195a90 into master May 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/launch-config-allow-substitutions-in-name branch May 15, 2019 20:06
@ivanpauno ivanpauno removed the in review Waiting for review (Kanban column) label May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hidmic hidmic hidmic approved these changes

Assignees

@ivanpauno ivanpauno

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ivanpauno @hidmic