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

Ability to clear current launch configurations #507

Closed
djchopp opened this issue May 10, 2021 · 6 comments
Closed

Ability to clear current launch configurations #507

djchopp opened this issue May 10, 2021 · 6 comments
Labels
help wanted Extra attention is needed

Comments

@djchopp
Copy link
Contributor

djchopp commented May 10, 2021

Feature request

Feature description

Somewhat related to #313 #501

Consider the following launch file setup:

  1. A base launch file has argument arg1 with default of base_arg
  2. Another launch file also has arg1 (same name, different purpose) with default of ext_arg and is included in the base launch file

Upon running the base launch file, arg1 within the included launch file will not have its default value of ext_arg, but rather it will be base_arg.

By default there is no scoping and the base value of arg1 will be passed on to the included launch file (overwriting the default). Surrounding the included launch file with PushLaunchConfigurations() and PopLaunchConfigurations() will not change this as these do not clear configurations, but only scope modifications to the existing context. Same goes for putting the incude in a GroupAction. One could push and UnsetLaunchConfiguration('arg_name') for each configuration they do not want, but this is tedious for included launch files with large numbers of configurations.

I would suggest a ClearContextConfiguration action that can be called after a PushLaunchConfigurations(), or as the first action in a GroupAction. Extremely simple implementation below, but could be expanded to have whitelist and blacklist functionality.

Is this feature wanted/inline with recommended design patterns? Is there another way to accomplish this without a new addition?

Its simple enough I could probably do the PR for it.

class ClearContextConfiguration(Action):
    """
    Action that clears the launch configurations from the context.
    """

    def __init__(self, **kwargs) -> None:
        """Create a ClearContextConfiguration action."""
        super().__init__(**kwargs)

    def execute(self, context: LaunchContext):
        """Execute the action."""
        context.launch_configurations.clear()
@hidmic
Copy link
Contributor

hidmic commented May 20, 2021

Upon running the base launch file, arg1 within the included launch file will not have its default value of ext_arg, but rather it will be base_arg.

I agree this is a problem, and an awfully subtle one.

Is this feature wanted/inline with recommended design patterns? Is there another way to accomplish this without a new addition?

Your proposal is straightforward. A ClearLaunchConfigurations would be in line with Push* and Pop*. If you could also update include and group actions to support opt-out scoping and explicit scope forwarding (i.e. to push/clear/pop by default) I think that'd make for a less surprising UX while keeping the current functionality around. Perhaps reverse that default to help a deprecation cycle for H-Turtle too. CC @ivanpauno @wjwwood

Would you be willing to take a stab at it @djchopp?

@hidmic hidmic added the help wanted Extra attention is needed label May 20, 2021
@djchopp
Copy link
Contributor Author

djchopp commented May 24, 2021

@hidmic Sure, I'll see what I can do.

@hidmic
Copy link
Contributor

hidmic commented Jun 14, 2021

Friendly ping @djchopp.

@djchopp
Copy link
Contributor Author

djchopp commented Jun 18, 2021

@hidmic I have an implementation for ClearLaunchConfigurations with forwarded configuration support. I also modified GroupAction to support this as well with the addition of an init argument forwarded_configurations: Optional[List[SomeSubstitutionsType]] that has the following behavior:

  • If the argument is null (forwarded_configurations=None), then it behaves the same as it has been and will not clear existing configurations.
  • If it is an empty list (forwarded_configurations=[]), all configurations are removed before appending the configurations defined in launch_configurations.
  • If the list is not empty (forwarded_configurations=[arg1, arg2, arg3...]), all configurations except those listed are removed before appending the configurations defined in launch_configurations. Note that the launch_configurations take priority and will overwrite any forwarded_configurations in the event the same key is defined in both.

As you suggested, the default for now is forwarded_configurations=None to give a deprecation cycle, but should be adjust to forwarded_configurations=[] for a "less surprising UX"

I am still working out how the IncludeLaunchDescription could have the same functionality. It does not have scoping functionality to begin with (where group action did), so it is a little less straight forward. I am working towards adding scope and forwarding, but am all ears if you have a better idea.

@hidmic
Copy link
Contributor

hidmic commented Jun 18, 2021

In principle it sounds great (though I will say that forwarded_configurations=None suggests the opposite 😅). I'd suggest you open a draft PR for me and other ROS 2 maintainers to take a look and perhaps chime in early.

Thanks for pushing @djchopp !

@hidmic
Copy link
Contributor

hidmic commented Oct 4, 2021

Closing as it was addressed by #515. Thank you @djchopp !

@hidmic hidmic closed this as completed Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants