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

added notes on prefix #399

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

Conversation

wkaisertexas
Copy link

prefix is part of kwargs, so it can be lost in the documentation

#398

`prefix` is part of kwargs, so it can be lost in the documentation

Signed-off-by: William Kaiser <[email protected]>
@@ -196,6 +196,7 @@ def __init__(
:param: remappings ordered list of 'to' and 'from' string pairs to be
passed to the node as ROS remapping rules
:param: ros_arguments list of ROS arguments for the node
:param: prefix command placed in front of executable on launch
Copy link
Member

Choose a reason for hiding this comment

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

We talked about this in the issue triage meeting, and the conclusion was that we should reference the base class that actually handles this argument, e.g. have a look at how Python does this for the subprocess API: https://docs.python.org/3/library/subprocess.html#subprocess.run

It's fine to give a brief description of the argument, but it should be separate from the other (actual) arguments and it should reference the place where it's actually documented, e.g. here: https://github.com/ros2/launch/blob/651f57451e4d0e93d055669764fcd588e2d9b1e6/launch/launch/actions/execute_process.py#L211-L216

It is indeed helpful to connect the dots here, but we don't want to duplicate too much documentation and risk them getting out of sync.

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.

2 participants