-
Notifications
You must be signed in to change notification settings - Fork 142
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 option to set output_format via environment variable #713
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Marc Bestmann <marc.bestmann@dlr.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm, though I would like to see additional documentation about this in the doc block, specifically about the existence of the env var and that passing a local argument for this will still override the env var.
Signed-off-by: Marc Bestmann <marc.bestmann@dlr.de>
a9311ad
to
46a3f81
Compare
Sorry for the delay, I was on a short vaccation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think that this should be done differently, and more flexibly. What I think should happen here is:
- If the environment variable is not specified and the user passes nothing in for
output_format
, we use the default ([{this.process_description.final_name}] {line}
). This preserves existing behavior. - If the environment variable is specified, we use that as the default instead.
- But if the user passes in
output_format
, that overrides the environment variable (or the default).
That way a user can use the environment variable to set all of the nodes to a particular format, but then have a single node where they do something else.
If we do this, I also think that the environment variable name should change to something like ROS_LAUNCH_OUTPUT_FORMAT
.
Thoughts?
@clalancette's proposal is fine with me. If we wanted to be really thorough, we could have what you suggest as well as an "override" env var which let you explicitly set it even if something else has been set in the code. This would be just for debugging situations I guess. But doing just what you suggested is sufficient probably. |
I changed the PR to the the proposed priority ordering of @clalancette. Please re-review it. |
friendly ping @clalancette |
Signed-off-by: Marc Bestmann <marc.bestmann@dlr.de>
1bbd1a7
to
13eaf6e
Compare
I also added a draft PR for documenting this PR ros2/ros2_documentation#4061 |
Signed-off-by: Marc Bestmann <marc.bestmann@dlr.de>
Co-authored-by: Tomoya Fujita <Tomoya.Fujita@sony.com> Signed-off-by: Marc Bestmann <marc.bestmann@posteo.de>
friendly ping @fujitatomoya @wjwwood |
friendly ping @wjwwood @fujitatomoya |
I would be really happy if we could move this PR forward. Its been almost a year since I created it. |
Can we somehow get forward with this PR, please? @wjwwood @fujitatomoya @adityapande-1995 @methylDragon @clalancette |
The failing checks are just an issue from the CI and I don't know why it says "changes requested", as I incorporated all changes that were requested. As far as I see, this PR is ready to merge. It just needs someone to give it another review. This PR also still blocks ros2/ros2_documentation#4061 |
This PR would close #664
I used a constant to get the default value instead of
self.__init__.__defaults__[6]
since adding any further arguments would break the code in that case.Any feedback is welcome.