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

Conditional before_commands #48

Open
nspielbau opened this issue Feb 9, 2024 · 0 comments · May be fixed by #51
Open

Conditional before_commands #48

nspielbau opened this issue Feb 9, 2024 · 0 comments · May be fixed by #51

Comments

@nspielbau
Copy link
Contributor

As discussed previously in person there might be a benefit of having conditional before commands, due to reduced session maintenance to keep e.g. a simulation and non-simulation session config up to date.

Let's take a stack where a roscore is running on a different machine in the non-sim case. In addition to not launching our simulation, we might also need to adjust some environment variables such as ROS_IP and ROS_MASTER_URI or ROS_DOMAIN_ID in ROS 2.
As we don't want to adjust them in the simulation case, we would need to either:

  • Write a new session file for non-sim use, rendering the window conditionality useless
  • Write if/unless versions for each window, making the file much more vulnerable for errors in the future when window commands change and are not updated properly.

For this I would suggest adjusting the before_commands as follows:

before_commands:
  - if: parameter
    command: COMMAND

the command: before the actual command could also be dropped for ease of use if a command is not conditional.

For this we would need to change the parsing of the common attributes and rearrange the sequence of parsing in session creation so parameters is parsed before common. Also we should probably adjust the example session file and move common behind parameters to reflect the change in session creation to the user (It would be very confusing if the parameters could be used before they are defined)

Any opinions?

@nspielbau nspielbau linked a pull request Dec 18, 2024 that will close this issue
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 a pull request may close this issue.

1 participant