Skip to content

add explanation for ROS_DISABLE_LOANED_MESSAGES.#2282

Merged
clalancette merged 4 commits intoros2:rollingfrom
fujitatomoya:feature-20211102-disable-loaned-message
Mar 22, 2022
Merged

add explanation for ROS_DISABLE_LOANED_MESSAGES.#2282
clalancette merged 4 commits intoros2:rollingfrom
fujitatomoya:feature-20211102-disable-loaned-message

Conversation

@fujitatomoya
Copy link
Copy Markdown
Collaborator

document for ros2/rcl#949

Signed-off-by: Tomoya Fujita Tomoya.Fujita@sony.com

Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

@wjwwood @clalancette requesting review.

@fujitatomoya fujitatomoya requested a review from wjwwood February 5, 2022 17:06
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

@wjwwood @clalancette requesting review.

Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I don't think that this information should be in this particular tutorial. This is the very first beginner tutorial, and we don't want to overload people with a bunch of advanced information about configuring their libraries.

I would suggest that this belongs either in one of the more advanced tutorials or in a How-To Guide.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Feb 9, 2022

I agree this seems like we're hijacking this beginner tutorial in order to turn it into a reference for all the environment variables that can be used to configure ros 2, when we should just have a reference page for that (not a tutorial).

Do we already have such a page?

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Feb 9, 2022

I could see putting a new page in either "How-To-Guides", something like "how to disable loaned messages", or in "Concepts", something like "loaned messages" which describes generally what they are and as part of that how to use this environment variable to control them.

Or we could have a reference page with all the environment variables you could use with ros 2, where one of them is this env var, but I don't know where that would live.

@clalancette what do you think?

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

thanks for the comment. I understand this is not really tutorial... 😅

a new page in either "How-To-Guides", something like "how to disable loaned messages", or in "Concepts", something like "loaned messages" which describes generally what they are and as part of that how to use this environment variable to control them.

sounds good to me.

@clalancette
Copy link
Copy Markdown
Contributor

I could see putting a new page in either "How-To-Guides", something like "how to disable loaned messages", or in "Concepts", something like "loaned messages" which describes generally what they are and as part of that how to use this environment variable to control them.

Yeah, that works for me too. I think it would be nice to have a page showing all of the possible environment variables that can be used with ROS 2, but I think that would be a long list, and is out-of-scope for this particular change.

@fujitatomoya fujitatomoya force-pushed the feature-20211102-disable-loaned-message branch from dd3d15b to 004d09f Compare February 18, 2022 19:39
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya force-pushed the feature-20211102-disable-loaned-message branch from 004d09f to 1f8ca65 Compare February 18, 2022 19:42
@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

@clalancette @wjwwood could you take a look again?

@fujitatomoya
Copy link
Copy Markdown
Collaborator Author

@wjwwood @clalancette

this is last one related to ROS_DISABLE_LOANED_MESSAGES, either of you can review?

Signed-off-by: Tomoya.Fujita <Tomoya.Fujita@sony.com>
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This looks good to me now, thanks.

@clalancette clalancette merged commit aa54b40 into ros2:rolling Mar 22, 2022
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.

3 participants